Migrate from NAN to N-API#471
Open
James Sigurdarson (jamiees2) wants to merge 4 commits intoconfluentinc:masterfrom
Open
Migrate from NAN to N-API#471James Sigurdarson (jamiees2) wants to merge 4 commits intoconfluentinc:masterfrom
James Sigurdarson (jamiees2) wants to merge 4 commits intoconfluentinc:masterfrom
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 :)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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