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
Merged

add support for node master #831

merged 1 commit into from Dec 18, 2018

Conversation

Flarna
Copy link
Member

@Flarna Flarna 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
Copy link
Collaborator

kkoopa 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
Copy link
Member Author

Flarna 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
Copy link
Collaborator

kkoopa 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.

@fivdi
Copy link

fivdi 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
Copy link
Collaborator

kkoopa commented Mar 7, 2019

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

@jwulf
Copy link

jwulf 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Node 12/master
4 participants