-
Notifications
You must be signed in to change notification settings - Fork 29
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
Replace node::MakeCallback
with AsyncResource call
#26
base: master
Are you sure you want to change the base?
Conversation
This PR fixed #24 |
I can confirm that this works. Using this pull request on Node 10 resolves the issue. @sandeepmistry Can we get this merged? |
Same here, I confirm that it fixes the problems on macOS. Would be amazing it if gets merged ASAP. |
Confirm this fixed compile for me on 10.3.4 OS X and 10.7.0 NodeJS |
Any news on this? |
@sandeepmistry could you please take a look? |
Please merge! 👍 |
@sandeepmistry sorry to ping you directly but a merge on this would be much appreciated! |
Not sure why exactly the owner hasn't merged this, but it may have something to do with all the unrelated changes in this PR. Things like:
I could be wrong, and the changes may actually be necessary for the fix, but in my experience, when a PR has extra unrelated "cleanup" changes, it can discourage the maintainer from wanting to merge it |
Since this repo appears to be abandoned, I took the liberty of cloning over the existing repo and implementing this PR for a package I called
Thanks so much for this @taoyuan 👏 |
node::MakeCallback
with AsyncResource call