N-API support for node-canvas

[jasongin]

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-canvas is one of the top native addons 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 migrate node-canvas to N-API. Your support and feedback is important in making this effort a success.

We (N-API team) have built a preliminary port of node-canvas on top of N-API as part of our effort to understand the API surface used by native modules. We used this port and the port of some other modules to determine which areas to focus on converting to N-API. This port has all of the NAN and V8-specific code removed and replaced by N-API equivalents. While it passes all the canvas unit tests and visual tests, it might not meet the quality and performance level to go in as-is. It would certainly be a springboard if someone on node-canvas was serious about doing the conversion to N-API. Please take a look if you're interested. I will be happy to answer questions about the port, point to documentation/code, etc.

If you'd like, you may join our WG meeting which happens every Thursday at 1:30 Eastern / 10:30 Pacific US time, to discuss any issues.

We'd assume you don't want to immediately take a dependency on a feature that still has "experimental" status. The best case scenario from our point of view might be if the node-canvas team could eventually maintain a branch that uses N-API, that is kept mostly up to date with development occurring in node-canvas/master. But I understand you might not be ready for that just yet. Of course we'd appreciate feedback about anything we can do to improve the development or migration experience.

[chearon]

It's been almost a year since you opened this and none of us have responded - sorry about that! It looks like a lot of work went into the proof-of-concept N-API port. And the code looks way better than NAN.

@zbjornson pointed out that this would simplify prebuilds a whole lot. It would save minutes of CI time. We only have to iterate the OS, (and on Linux maybe extra arches and libc implementations, but nbd since Linux is the fastest CI by a long shot). Currently I think that's the biggest issue this solves since I've only seen a small number of issues with the Module version mismatch error.

The downside is that N-API still looks like it's experimental status. So we wouldn't benefit much from getting it working with the current master other than saving us some future work. Are any other native node libraries out there using N-API yet?

[jasongin]

@mhdawson What is the latest on the experimental status of N-API?

[mhdawson]

It is non-experimental in master. We are now working on backports for 8.X and 6.X

[zbjornson]

Now that Node.js 6.x is EOL, N-API is non-experimental in all supported releases, so it finally makes sense maintenance-wise to move forward with this I think!

[mhdawson]

@zbjornson great to hear that :)

[AlexVestin]

Is https://github.com/jasongin/node-canvas/tree/napi the most recent version to use for n-api bindings @jasongin ?

[jasongin]

Yes that's the most recent as far as I know, but as you can see from the commits I haven't worked on it since 2 years ago, so it's a very out-of-date version of node-canvas.

[AlexVestin]

Yeah, I will probably do an effort to make an updated version, but I thought I'd check if it had been done already.

[DePasqualeOrg]

I'd love to use node-canvas in multiple worker threads, which is currently not possible, and it seems like adding N-API support would finally allow this. Is there any chance this will be implemented soon?

https://github.com/nodejs/node/blob/master/doc/api/addons.md#worker-support

[anna-rmrf]

Yeah, using node-canvas in worker threads should be very good

[caldwellc]

Any updates on this? It would be great to run this in a node worker thread!

[Brooooooklyn]

@napi-rs/canvas is almost done here, based on N-API and skia, Including the Async API.

[brianhvo02]

Will there be any updates to the extent of NAPI or is this not something that will be pursued?

[LinusU]

I'm very open to this and have converted my other native modules to NAPI. However, it's quite a bit of work and I don't think that it's possible to do it incrementally?

If that is possible, I would be very happy to merge the groundwork asap, and then accept PRs for each small part of the native part 🎉

Otherwise we probably need to plan a bit on when to do it so that we don't get conflicts with everything else...

[GitMurf]

Is there any update on Canvas being ported to NAPI? Currently Canvas is not compatible with Electron due to Nan so if Canvas could get ported to NAPI that would be huge for the Electron world.

If there is no “official” plans to port, does anyone know if there is a newer fork that has ported to NAPI since @jasongin did it here? https://github.com/jasongin/node-canvas

[GitMurf]

Checking in to see if any updates or suggestions from anyone or most importantly any interest in porting to node addon api so that canvas can work with Electron?

[chearon]

I'm definitely interested in picking this up some time this year. On top of Electron and worker threads, this would mean we can support Bun and it's a key part of making prebuilds less of a pain.

Update: I've started work on N-API. It is my top priority when I have free coding time.

[GitMurf]

@chearon i saw this recent comment in another issue (#2155 (comment)). Are you referring to converting from NaN to the node addon api as described here? 🤞🤗

[chearon]

Yes! I'm something like 30% done.

[GitMurf]

@chearon Very excited for this! Thank you! I did a bunch of research in the past and made a few attempts at converting using the node addon API but ultimately got stuck... but I did learn a lot so if you get stuck or have any questions let me know and maybe I can help and/or dig up some of my old notes or attempts at conversion to provide any support. Let me know!

[GitMurf]

Also @chearon I made an official request to the node addon team a few months ago to take a look and they mentioned they were interested but haven't found the time yet. @NickNaso from their team mentioned he planned on taking a look so maybe he could support you if you have any questions!

Here is the GH Issue over there: nodejs/node-addon-api#1250

[chearon]

Very excited for this! Thank you! I did a bunch of research in the past and made a few attempts at converting using the node addon API but ultimately got stuck... but I did learn a lot so if you get stuck or have any questions let me know and maybe I can help and/or dig up some of my old notes or attempts at conversion to provide any support. Let me know!

Much appreciated! It would be most helpful if you could be a reviewer on the PR when I submit it since you already have experience. Review is going to be a slog so I plan to list out areas where I had to make my own calls. I haven't gotten stuck yet but I had to stop and learn about libuv/async callbacks for a while. Do you remember what you got stuck on?

Also @chearon I made an official request to the node addon team a few months ago to take a look and they mentioned they were interested but haven't found the time yet. @NickNaso from their team mentioned he planned on taking a look so maybe he could support you if you have any questions!

Cool, thanks. Come to think of it, the one thing I stumbled on is our CHECK_RECEIVER calls. They don't seem necessary with N-API but I can't figure out why they were necessary with NAN to prove it. May ask about it when I can formulate a question better.

[GitMurf]

It would be most helpful if you could be a reviewer on the PR when I submit it

Happy to when ready!

I had to stop and learn about libuv/async callbacks for a while. Do you remember what you got stuck on?

This is actually one of the major areas I got stuck on 🤣 It has been a while since I was in the canvas mindset ;) but another area I was having trouble with off the top of my head was the sub classing / inheritance for the Backend and getting that to work with N-API. For the PDF, Image and SVG backends.

Come to think of it, the one thing I stumbled on is our CHECK_RECEIVER calls.

Ahhh yes, again off the top of my head these were the macros right? I remember spending quite a bit of time trying to figure out what was the purpose there too 🤣 If I remember correctly, I just basically converted it to work and didn't worry about the outcome / usage of it because I couldn't even figure out the point of it, haha!

[GitMurf]

@chearon I just looked through my notes and definitely did some research on this but unfortunately I cannot find any notes on what I settled on. But at least can confirm we both ran into the same "hmmm, I need to research this to figure out the purpose" haha.

[GitMurf]

@chearon take it with a grain of salt because I was treading water at this point trying to figure this stuff out as I hardly knew any c++ and was trying to learn the node addon api at the same time. But it looks like this is what I came up with and if I recall, I THINK I ended up determining that this check has something to do with making sure you were properly calling / instantiating Canvas correctly and not trying to access things directly on the constructor? Or something like that? Not sure if that makes sense, and my memory is very fuzzy on it.

#define CHECK_RECEIVER(prop, returnResult) \
  /* cout << "CHECK_RECEIVER for prop: " << #prop << endl; */ \
  if (!info.This().As<Napi::Object>().InstanceOf(Canvas::constructor->Value())) { \
    cout << "CHECK_RECEIVER for prop: " << #prop << " failed" << endl; \
    Napi::TypeError::New(info.Env(), "Method " #prop " called on incompatible receiver").ThrowAsJavaScriptException(); \
    returnResult; \
  } else { \
    cout << "CHECK_RECEIVER for prop: " << #prop << " passed" << endl; \
  }

[GitMurf]

To my points above, maybe it has something to do with the changes in how the module is imported and instantiated and the whole "using new" keyword? See in the changelog a couple potential items related to why there is a CHECK_RECEIVER? https://github.com/Automattic/node-canvas/blob/master/CHANGELOG.md#200

[GitMurf]

@chearon yippy! Nice work! I see the PR :) Thank you for your hard work!

[GitMurf]

@chearon curious what you found on the CHECK_RECEIVER stuff? were my hunches above correct? did you figure out why they were necessary?

[chearon]

No problem, and sorry I forgot to answer with my findings:

area I was having trouble with off the top of my head was the sub classing / inheritance for the Backend and getting that to work with N-API. For the PDF, Image and SVG backends.

This was the first thing I got stuck on too. You can only use ObjectWrap for a derived class, so that's what I did. To instantiate an ObjectWrap class, you have to go through the JS object (via Napi::Object* jsObject = jsObject.New()) and unwrap it (via ImageBackend* backend = ImageBackend::Unwrap(jsObject).

curious what you found on the CHECK_RECEIVER stuff? were my hunches above correct? did you figure out why they were necessary?

It's there so you can't do Canvas.prototype.width which would try to unwrap the this pointer as a Canvas. I didn't think it should be necessary since node-addon-api is the one doing the unwrapping: it should do the check. Turns out I was right about that, but node-addon-api is missing a null check when you use maybes instead of exceptions. That's one of two PRs I'm going to make to node-addon-api today. Until then, we do need to verify the this arg.

[GitMurf]

@chearon awesome, thanks for those updates. Very helpful! It would be awesome to also see what the node addon team thinks and if they have any comments / feedback for your PR (#2235). cc @mhdawson

This has been a few years in the making, and canvas is a popular library, so I think @mhdawson and team will be interested to see this completed :)

[mhdawson]

Great to hear this and your description in the PR.

[kotasudhakar]

When will this get released?