Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement progress to NanAsyncWorker. #207

Merged
merged 1 commit into from
Jan 14, 2015
Merged

Conversation

brett19
Copy link
Contributor

@brett19 brett19 commented Oct 26, 2014

This builds on NanAsyncWorker by providing a NanAsyncProgressWorker which can dispatch progress information back to the originator.

@brett19 brett19 changed the title #197: Implement progress to NanAsyncWorker. Implement progress to NanAsyncWorker. Oct 26, 2014
@brett19 brett19 mentioned this pull request Oct 26, 2014
@brett19
Copy link
Contributor Author

brett19 commented Oct 26, 2014

This should have everything we wanted from the discussion against the previous PR by @kkoopa.

*
* Copyright (c) 2014 NAN contributors
*
* MIT +no-false-attribs License <https://github.com/rvagg/nan/blob/master/LICENSE>

This comment was marked as off-topic.

@brett19
Copy link
Contributor Author

brett19 commented Oct 26, 2014

Updated PR with fixes for @kkoopa's comments, as well as a fix to resolve a race condition I discovered.
P.S. Had to add a Destroy() method to handle destruction virtually, as NanAsyncProgressWorker has 'pre-work' necessary before its safe to delete the worker.

This was referenced Nov 4, 2014
@jonathanong
Copy link

i'm testing this and it's working pretty well. i don't know much about C++ and stuff but i got it to work with @rvagg's help and it was pretty straight forward.

so... a merge please!!!

@rvagg
Copy link
Member

rvagg commented Jan 9, 2015

This is slated for a 1.5 (#216) but it does need some docs I believe.

@brett19 do you want to add some docs to the README for this?

@kkoopa what do we need for a 1.5?

@brett19
Copy link
Contributor Author

brett19 commented Jan 9, 2015

I am unfortunately extremely busy at the moment, I'll try to get it done next week.

@kkoopa
Copy link
Collaborator

kkoopa commented Jan 12, 2015

What we need and what we want are two possibly separate things. Fixes and such for broken code or new version incompatibilities are what is strictly needed. That includes #223 and a new thing: v8::V8::ContextDisposedNotification has moved to the current isolate. Additionally, what's already in #216 should also be in the new version.

That should be it. Remaining improvements are probably better off for later versions. Oh yeah, we should test against io.js too.

@kkoopa kkoopa merged commit 8d6a160 into nodejs:master Jan 14, 2015
@jonathanong
Copy link

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants