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. |
Tim Hall (timhall)
left a comment
Tim Hall (timhall)
left a comment
There was a problem hiding this comment.
Was running this branch through the e2e test suite and some were failing with Invalid argument whenever the opaque argument is a primitive (string / number / boolean). Suggested a fix that boxes the value in an object before referencing it and unwrap it in the delivery report callback.
|
Would love to see this as well. We're running Deno in production and this would let us adopt this library! |
|
Hi James Sigurdarson (@jamiees2) , Tim Hall (@timhall) : Is this something we can merge ? |
# Conflicts: # package-lock.json # package.json
| producer.produce('test', null, Buffer.from('value'), null, null, 'opaque'); | ||
| }); | ||
|
|
||
| it('should preserve non-string primitive opaque values', function(done) { |
There was a problem hiding this comment.
added a new test as per Tim Hall (@timhall)'s comments, there wasn't one to cover primitive values
|
Apologies here, vacations caused me to put this on hold for a bit. I've updated the code off of the latest master, and addressed the review comments, and rerun all tests and validated the behavior, thank you! Robert Yokota (@rayokota) (as a recent PR author), could you let me know if this is something we can see ourselves merge, or if I'm barking up the wrong tree here? I say this as I would just like to know how much it's worth continuing to maintain this PR, and as we've already built our Kafka pipeline assuming this is the direction we're heading, we'd like to explore our options if it isn't. Again, let me know if there's anything I can do to help smooth things along, or any process that I'm missing that I can engage with. As mentioned, we have been successfully operating this PR in production (on node-caged) since March 2026 at scale. |
5 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