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

add support for node master #831

Merged
merged 1 commit into from Dec 18, 2018

Conversation

@Flarna
Copy link
Member

commented Dec 17, 2018

This PR allows to use NAN with current Node on master and should fix several fails in CITGM which use NAN.

Not sure if this should be really included in NAN at this time as Node plans to do further updates of V8 till v12 is released (see nodejs/node#25060 (comment)).

There is still a self deprecation warning in v8 left but I think this should be addressed inside v8 or in the patched v8 variant included in node.

Refs: nodejs/node#25060
Fixes: #829

@kkoopa

This comment has been minimized.

Copy link
Collaborator

commented Dec 18, 2018

Looking good. Not too many breaking changes so far. Will hold off until Node 12 gets closer to release. Thank you.

@Flarna Flarna force-pushed the Flarna:node-master branch from d759c88 to 8135021 Dec 18, 2018
@Flarna

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2018

Rebased to 2.12.1.

I'm fine with waiting.

I think there are quite some people out there which would be happy with a nan release working with node master. But it's up to them to convince you to take this maintenance burden 😏.

Maybe someone (or even myself) finds some time to port nan to use napi. This would help a lot in future but not sure if it is that easy. It would be for sure a good test for napi completeness.

@kkoopa

This comment has been minimized.

Copy link
Collaborator

commented Dec 18, 2018

I think there are quite some people out there which would be happy with a nan release working with node master. But it's up to them to convince you to take this maintenance burden .

Maintenance burden indeed. Unfortunately, I do not have time to follow a volatile moving target with actual releases, since every release comes with a risk of something unintentionally breaking. However, merging to master would not hurt. Whoever wants to be on the bleeding edge can easily do so then.

Maybe someone (or even myself) finds some time to port nan to use napi. This would help a lot in future but not sure if it is that easy. It would be for sure a good test for napi completeness.

I had originally thought of doing precisely that, but shelved the plans due to lack of time and motivation. A common feature set could likely be mapped onto NAPI, but AFAIK there are be some (rare) V8-specific features which arguably do not fit in napi, e.g. the garbage collection API.

@kkoopa kkoopa referenced this pull request Dec 18, 2018
@kkoopa kkoopa merged commit d113c02 into nodejs:master Dec 18, 2018
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@Flarna Flarna deleted the Flarna:node-master branch Dec 18, 2018
@fivdi

This comment has been minimized.

Copy link

commented Mar 6, 2019

@kkoopa do you think it's time to release a new version of nan with the modifications from this PR? Node 12 is scheduled to be released in April (https://github.com/nodejs/release.)

@kkoopa

This comment has been minimized.

Copy link
Collaborator

commented Mar 7, 2019

Yes, it is indeed time. Thank you for reminding me.

@jwulf

This comment has been minimized.

Copy link

commented Apr 25, 2019

It's out now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.