-
Notifications
You must be signed in to change notification settings - Fork 697
Implement support for Node 12 #1696
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
Conversation
|
Hi there! Thanks for working on this! Can you modify the travis.yml and appveyor.yml files to specify tests on node 12? |
implausible
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this is pretty straight forward and easy to review. I'm excited to get this running in CI to verify your work!
|
I've modified both the files but now it's failing, and I'm not entirely sure why, it mentions things that aren't in the generated src folder like BooleanValue..? |
|
So it was just nan itself being out of date.. I also updated node-pre-gyp for good measure. (There's an updated node-gyp as well but 4.0.0 is quite recent compared to the node-pre-gyp that was being used) |
|
What a time to be alive... |
|
All should be good now, no more depreciations that I can see or find. Hopefully this will help get 0.25.0 out of alpha stage - it's already really stable as far as I can tell.. |
|
@implausible is this all good now? It passes all the CI checks (cleanly too.. 😀) |
implausible
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking care of this! I will get this into the next alpha. Also, we are looking to close out the alpha for 0.25.0 before month is out.
|
Oof, this seems to have introduced instability into the test suite. These tests are failing on master and on several unrelated PRs: |
|
That’s very strange. It seems to affect three Rebase and two Remote tests. I can’t see why the changes would be causing those to be flaky. |
|
Bit of a long shot but could it be because the enhanced tests still run on Node 8? Maybe try them on 10, the new Nan can’t be optimised (at the very least) for 8? |
|
Definitely worth considering. I've also re-read this entire PR and also can't determine what did it. I am going to double check that these tests hadn't been failing randomly before this PR as well. I don't recall seeing these tests fail in the past. |
Fixes #1680 - tested on Node 10.16.0 and Node 12.6.0