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

fix: propagate async context #199

Merged
merged 2 commits into from Apr 3, 2018
Merged

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Feb 23, 2018

Start with Nan 2.9.0 certain variants of Nan::Callback::Call are
deprecated. Overloads that preserve the async context are preferred
instead.

We associate a Nan::AsyncResource corresponding to a FSEvents instance.
This captures the async execution context which is restored when the
async callback is called.

For more information see https://github.com/nodejs/nan/blob/HEAD/doc/node_misc.md#nanasyncresource.

EDIT: this fixes the compile warnings that are presently displayed:

In file included from ../fsevents.cc:85:
../src/methods.cc:14:12: warning: 'Call' is deprecated [-Wdeprecated-declarations]
  handler->Call(3, argv);
           ^

Start with Nan 2.9.0 certain varaiants of Nan::Callback::Call are
deprecated. Overloads that preserve the async context are preferred
instead.

We associate a Nan::AsyncResource corresponding to a FSEvents instance.
This captures the async execution context which is restored when the
async callback is called.

For more information see https://github.com/nodejs/nan/blob/HEAD/doc/node_misc.md#nanasyncresource.
@bnoordhuis
Copy link
Contributor

The v0.12 build error is ibmdb/node-ibm_db#276 (comment). I guess today is the day we say goodbye to v0.12.

Ali, can you apply this patch?

diff --git a/.travis.yml b/.travis.yml
index f98b0a6..1fe7d11 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -10,7 +10,6 @@ env:
     - NODE_VERSION="v6"
     - NODE_VERSION="v5"
     - NODE_VERSION="v4"
-    - NODE_VERSION="v0.12"
     - NODE_VERSION="v0.10"
 
 before_install:

@es128 How do you feel about dropping support for v0.10 as well?

With new enough compilers, V8 headers corresponding to 0.12 no longer
compile. For more details see
https://github.com/nodejs/nan#compiling-against-nodejs-012-on-osx
@ofrobots
Copy link
Contributor Author

Thanks for reviewing! Added commit to drop builds against Node 0.12.

@ofrobots
Copy link
Contributor Author

ofrobots commented Mar 2, 2018

LMK if anything more is needed from my side (e.g. dropping 0.10 as well).

@es128
Copy link
Contributor

es128 commented Mar 2, 2018

Seems like we need to bump major to publish this regardless, but if 0.10 will still work it seems like we should leave it alone to reduce impact on users. For the population that's stuck on old versions, probably a lot more of them on 0.10 than 0.12.

Other thoughts?

@ofrobots
Copy link
Contributor Author

ofrobots commented Mar 2, 2018

@es128: I am not sure this deserves to be semver major. The 0.12 builds on OS X would already be broken if you have a new enough compiler; or they would be fine if you have a old enough compiler. IOW, this change doesn't cause the breaking change, it merely drops the platforms that are known not to be compilable in the CI anymore from the CI.

@es128
Copy link
Contributor

es128 commented Mar 2, 2018

Since we have been providing pre-built binaries, not doing that anymore will be a breaking change for any users on 0.12.

@ofrobots
Copy link
Contributor Author

ofrobots commented Mar 2, 2018

Ah, I see. I guess what you are saying is that there is no means at present to release a fix on the current semver major line; so you need to bump major independently and bundle this change in.

@bnoordhuis
Copy link
Contributor

I'm a little torn. v0.12 has been out of support for more than a year now and the ecosystem has largely moved on. Users stuck on <= v0.12 have presumably learned to pin their dependencies by now.

I don't wholly disagree with the premise that we should bump major when we drop support for v0.12 but that means downstream users won't get updates until they update their package.json. And since inertia is a powerful force, that might be a while.

@es128
Copy link
Contributor

es128 commented Apr 3, 2018

Sorry for the long delay.

So it's a bit of deviation from strict semver, but the pros would seem to outweigh the cons. Let's bring this in without bumping major.

@es128 es128 merged commit 22beb2b into fsevents:master Apr 3, 2018
@es128
Copy link
Contributor

es128 commented Apr 3, 2018

My latest thinking after a bit of further discussion in #201 (comment) is to bump major here, but then just bump patch in chokidar to get the fix propagated easily. Pretty much all usage of fsevents goes through chokidar.

Feel free to chime in with alternate opinions.

@ofrobots ofrobots deleted the async-resource branch April 3, 2018 23:30
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.

None yet

3 participants