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.

Sign up for GitHub

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

Jump to bottom

Add node 12 support #850

Closed
wants to merge 2 commits into from
Closed

Add node 12 support #850

wants to merge 2 commits into from

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Apr 25, 2019

No functional change, just update readme, CI and add NODE_12_0_MODULE_VERSION.

Refs: #849

mkrufky
mkrufky approved these changes Apr 25, 2019
View reviewed changes
Copy link
Collaborator

@mkrufky mkrufky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me - thank you 👍

@mkrufky
Copy link
Collaborator

mkrufky commented Apr 25, 2019

hmmm. It looks like appveyor hasn't caught up to node 12 yet. https://ci.appveyor.com/project/RodVagg/nan

@Flarna
Copy link
Member Author

Flarna commented Apr 25, 2019

Should I revert the change for appveyor till this is available?

@mkrufky
Copy link
Collaborator

mkrufky commented Apr 25, 2019

Being that the PR isn't actually necessary for building modules against Node.js 12, I'm inclined to simply wait for Appveyor to include support for Node 12 before merge. It'll likely happen within days...

@AlonMiz
Copy link

AlonMiz commented Apr 30, 2019

Any news? Thanks.

@Flarna
Copy link
Member Author

Flarna commented Apr 30, 2019

Refs: appveyor/ci#2921

msimerson added a commit to msimerson/nan that referenced this pull request May 2, 2019
@msimerson
add windows test support to Travis
e51c364 
This is test PR, adding windows build support to Travis. It's an alternative for nodejs#850 to waiting for appveyor support.  IME, Travis windows builds are a fair bit slower than appveyor, but I've also had plenty of false positives on appveyor tests, so it's a mixed bag.
@msimerson msimerson mentioned this pull request May 2, 2019
Closed
@msimerson
Copy link

For anyone that's anxious for this to be merged, I've created a first step in #853 that adds Travis' windows support. It'll need a little help though to get it working.

@jdalton jdalton mentioned this pull request May 6, 2019
Closed
@bnoordhuis
Copy link
Member

Thanks, landed in e512f92.

@bnoordhuis bnoordhuis closed this May 6, 2019
@fanatid
Copy link
Contributor

fanatid commented May 6, 2019

@bnoordhuis thanks for merge, can you publish new version on npm too?

@bnoordhuis
Copy link
Member

No real need. Except for the NODE_12_0_MODULE_VERSION macro there are no functional changes and consumers of nan normally have no need for that macro. (If you think you do, please explain why.)

@Flarna Flarna deleted the node-12 branch May 6, 2019 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkrufky mkrufky mkrufky approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Flarna @mkrufky @AlonMiz @msimerson @bnoordhuis @fanatid