N-API support for node-serialport

[mhdawson]

The recently announced Node 8 has a new experimental feature called N-API which is aimed at reducing maintenance cost for node native addons. Checkout this blog for more details on its benefits.

node-serialport is one of the top 30 native modules by download dependency count, and in order to help the Node.js community make the important transition to N-API, we are hoping you will be able to work with us in order to port X to support N-API. Your support and feedback is important in making this effort a success.

I am part of the N-API working group and and would like to talk to you about how you can can get started with N-API and what help we might be able to provide. I'm available to talk on the phone individually if you'd like. Alternatively, if you prefer, feel free to jump in our WG meeting which happens every Thursday at 1.30 Eastern / 10.30 Pacific US time, to discuss any issues.

Here’s a video of N-API in action
https://www.youtube.com/watch?v=nmXhJ88nZsk

[reconbot]

This is great. Once it supports LTS nodes this will make life a lot easier for a lot of people.

[hanfengcan]

great !

[mhdawson]

@reconbot can we set up a time to have a quick chat ?

[reconbot]

hit me up via email @mhdawson

[mhdawson]

sent to your wizard email listed in github profile.

[mhdawson]

This is my understanding of our conversation today:

We talked today about Node Serial port. Francis is happy to have a branch in the repo and help maintain once there is a port.

Francis sees definite benefits in that today the npm needs to support downloading because there has to be a different binary for every version of Node.js and the sum of those are too big to bundle into an npm. With a single binary bundling would be possible.

The plan is for me to take a first cut at porting, Francis is available to support us by answering questions etc. Once we have an initial cut we'll get back together to get it into a branch in the repo and figure out the plan going forward.

[reconbot]

Now is the time to go head with what's in master I think 5.0.0-beta6 will probably be the last beta. =)

[mhdawson]

Starting to work on the port here https://github.com/mhdawson/node-serialport/tree/n-api. Have just started so have only done a small part of the conversion so far.

[mhdawson]

One note is that I saw a few tests failing before I started, notably those that required changing the baud rate. I commented those out and other than those 3-4 all were passing using a connected arduino and linux. I figure as long as the same tests pass after the N-API port that will be a good starting point.

[reconbot]

Looks good so far =)

[joegoggins]

👍 !

We (at Particle) are really excited about the prospect of simplifying and improving reliability of node-serialport installations across the various platforms our particle-cli npm module is designed to run on. This has been a big pain point for our community. Am I correct in thinking that this work will help on this front?

I'm assuming the answer is yes. If not, would be curious to know where I got the wrong assumption.

We'll be watching progress of N-API and this issue, thanks for taking this on, please don't hesitate to reach out to me or @monkbroc if there is potential technical work we could do to help with this.

[reconbot]

This will help for sure. I switched us to prebuild with the last major bump which also seems to have helped installation and provides us with a lot more prebuilt binaries than we had before. (Over 50!) The N-API (currently only supported in node 8+) when stabilized and backported will allow us to have a single binary per platform for all or most versions of node.

Another side effect of prebuild is much smaller binaries, it's now feasible to package them with serialport and not worry about downloading anything. I'll be pushing for "binding packages" so the core serialport module doesn't rely on c++ itself and instead relies on installing the right binding package, those could include all necessary packages for a platform.

All this is neither here nor there if n-api takes over.

[mhdawson]

I've not made as much progress as I'd like on this (just lack of time, its been a busy last few months for me), @joegoggins if you would like to collaborate on the port lets touch base and discuss how we do that.

[joegoggins]

Hey there @mhdawson thank you kindly for reaching out to collaborate. Despite our keen interest in seeing this project move forward, we are actually in the same boat--we don't have extra engineering muscle to throw down on this right now. Additionally, in grok-ing the commits you introduced in your n-api serialport fork, I don't know if we have anyone here that would be able to contribute quickly and productively to this project.

All of that ^^ could change though; so please reach out the next time you dig deep into this or want someone to alpha test your fork. In the meantime, let's hope someone else on the interwebs steps up with the resources to you help push this forward.

Cheers 🍻 !

[mhdawson]

@joegoggins thanks for the response. I do hope to get back to this soon and I'll keep this issue up to date so you can see where it is at if there are changes on your side.

[reconbot]

Hey everyone, what's the status of this?

[reconbot]

And does this prebuild issues effect it at all? prebuild/prebuild#220

[mhdawson]

Unfortunately, I never squeezed in time push it forward. If node-serialport uses prebuild then that discussion/PR will help when it comes time to build binaries for N-API versions.

[reconbot]

I understand, It is a lot of work to maintain a separate binding package that used n-api. I recently switched us to only supporting LTS. So.. it would pay off eventually but there isn't a rush on my end.

…

On Wed, Aug 29, 2018, 9:13 AM Michael Dawson ***@***.***> wrote: Unfortunately, I never squeezed in time push it forward. If node-serialport uses prebuild then that discussion/PR will help when it comes time to build binaries for N-API versions. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1186 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABlbrbalsmooIsK_9w0SKezexmNDEDSks5uVpOGgaJpZM4Nx7D_> .

[reconbot]

So node 6 is EOL Today 🎉

I had a call with the N-API working group yesterday where they guided my hand a bit. The biggest takeaway is that I can't use node's libuv, as that's not ABI stable. (or it isn't guaranteed but is stable and maybe we can't rely on that?) I love libuv but as far as I know there's not much I can do, other than move away from it. The steps as I see it;

• We can consider node-addon-api vs the c function calls.
• We can replace all use of uv_work_t with napi_async_work
• We have to rewrite serialport not use use uv_poll_t on Darwin and Linux
• We can keep prebuild which we use for publishing and downloading the binaries is already N-API aware, so we need to tell it we are now N-API compatible.

[reconbot]

Found nodejs/abi-stable-node#361

[reconbot]

Which now says we can probably rely on uv_poll_t remaining abi stable with v2 =)

[hugo-a-garcia]

I tried a simple example of reading from the serial port and it worked but when I try to call the same port reading file from a worker then I get and error which seems related to serialport still using NAN???

FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope
1: 0x9bcac0 node::Abort() [node]
2: 0x9bdc56 node::OnFatalError(char const*, char const*) [node]
3: 0xb18cba v8::Utils::ReportApiFailure(char const*, char const*) [node]
4: 0xc932aa v8::internal::HandleScope::Extend(v8::internal::Isolate*) [node]
5: 0xe6455d v8::internal::JSReceiver::GetCreationContext() [node]
6: 0xb29ed8 v8::Object::CreationContext() [node]
7: 0x934303 node::MakeCallback(v8::Isolate*, v8::Localv8::Object, v8::Localv8::Function, int, v8::Localv8::Value, node::async_context) [node]
8: 0x7f6305df23f2 EIO_AfterOpen(uv_work_s) [/home/hugo/node/projects/worker-lmax/node_modules/@serialport/bindings/build/Release/bindings.node]
9: 0x12aafa5 [node]
10: 0x12af3f1 [node]
11: 0x12c1438 [node]
12: 0x12afd7b uv_run [node]
13: 0xa002c7 node::NodeMainInstance::Run() [node]
14: 0x993cd8 node::Start(int, char**) [node]
15: 0x7f630cd7eb97 __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6]
16: 0x932bf5 [node]
Aborted (core dumped)
hugo@yukiyu:~/node/proj

[GazHank]

Hi All,

I would like to understand the level of interest/ support for this feature request. I know this issue is quite old, but I think it is still relevant and worthwhile.

Could you please let me know if you would like to support or contribute to migrating to N-API?

In particular I have local version where I have ported the core (non platform specific functionality) over to napi, and am working through the platform specifics, for windows at the moment, and plan to look at Linux next.

At the moment it feels a little hacky, but the happy path is working so I'm encouraged to progress. If people are interested in getting involved then I would be happy to share the work in progress for review, feedback and any guidance on best practice or examples of other projects which could act as good reference implementations (license permitting).

Don't worry if you don't have time to support, I'll continue to work on this in my garage (/local repo); but if other are able to help them I'll try to share updates so we can collaborate on this

[reconbot]

If you do a draft pr I'm more than happy to do review. This is still a very important feature.

[GazHank]

I've created #2305 for this. Please note this only includes the architecture agnostic changes as the other stuff is still WIP, but I would be very keen for any review, feedback, comments and suggestions. To build and test at the moment you need to stub out the arch specific functions; as such a pure code review is probably easier.

@mhdawson If you or any of your team of NAPI experts are able to review or help then it would be greatly appreciated

[mhdawson]

@GazHank created nodejs/abi-stable-node#430 so that the team is aware of the ask and we can see if anybody has some time to review/help.

[reconbot]

Thanks Michael

[NoahAndrews]

This can be closed now. 🚀