Migrate from NAN to N-API#471
Conversation
This includes revamping a lot of call sites in order to include the NAPI env, where necessary, and a lot of type modification in order to get the build working.
| } else { | ||
| Nan::Utf8String utf8_value(value.As<v8::String>()); | ||
| string_value = std::string(*utf8_value); | ||
| string_value = value.ToString().Utf8Value(); |
There was a problem hiding this comment.
This may be a bug - I preserved the same behavior from NAN, but this means that an undefined/null value gets passed as the string "undefined" or "null".
I don't want to fix that bug here, but just wanted to call it out since I noticed it :)
|
Hey Robert Yokota (@rayokota) and Emanuele Sabellico (@emasab), tagging you as recent members who have been approving PRs in this repository, and I haven't seen much engagement. What does the process for merging this PR look like? Is there anything I can do to help smooth things along? We have been successfully running this PR in production for about 2 months now, and haven't had any issues, and I'd love for us to not be on a fork forever :) |
|
would love to see this PR implemented. Running bun on our infra and want to be able to implement this library. |
2 participants
What
This converts the C++ layer from NAN to N-API. This builds on #281 , but with an up-to-date master, and gets the build and all tests working and passing.
Moving to N-API is pretty important to enable more widespread support for this library, in order to use it with other runtimes, e.g bun/deno. N-API is the current official recommendation by the Node.js project over NAN. N-API also abstracts away v8 internals, leading to fewer node version checks.
This is a fairly large PR since ripping out NAN is a pretty core change to the codebase. I tried my best to keep the changes minimal, but there are some structural changes that were necessary, mainly that some code needed to be converted to templates, and some code had to be moved around to obey the N-API object structure.
Checklist
References
JIRA:
Test & Review
I ran all tests
make test, andmake promisified_test. I did not getmake e2epassing, since it doesn't seem to be passing on the master branch.I ran the example performance test to verify that performance is the same:
I ran the same benchmark off of the master branch (NAN version), and got the following numbers:
Note that these benchmarks were just run on my laptop, and not in a CI harness or anything so run-to-run variance is higher. My conclusion is that there is no performance impact.
I intend to start using this version of the library myself in a project I'm working on with real data.
Open questions / Follow-ups
I'll tag Milind L (@milindl) and Charles Lowell (@cowboyd) from #281