Electron 21 breaks api compatability with some modules

[Julusian]

Blog post about the change: https://www.electronjs.org/blog/v8-memory-cage, coming to v21, not 20 like the post states.

Essentially calls to napi_create_external_buffer fail with

[827133:0924/184053.372625:ERROR:node_bindings.cc(146)] Fatal error in V8: v8_ArrayBuffer_NewBackingStore When the V8 Sandbox is enabled, ArrayBuffer backing stores must be allocated inside the sandbox address space. Please use an appropriate ArrayBuffer::Allocator to allocate these buffers.

To me this goes directly against the promises of node-api, as 5 of 5 of the native modules I have contributed to will not work in electron 21 without changes.

I don't know if people here have any sway over at electron, but as they currently support the same node-api and will soon be using a subtly different version of node-api (which doesnt have its own documentation), it might be a good idea to talk with them about it

[mhdawson]

We had some good discussion and talked about what we might be able to do that would be useful.

The only thing we have in mind so far might be to add a #define that can be used to exclude napi_create_external_buffer so that it is easier for package authors to know if there are going to be problems on runtimes that no longer suppoirt it.

We'll think more about it and discuss again next week.

I don't think we have any hope of change electron's mind in terms of the decision they took.

[Julusian]

Yeah, I think that a define would be a good help here. That will at least help those who are aware enough to enable it.
Hopefully a discussion can be had over whether node-addon-api should enable that define by default in a future release.

I don't think we have any hope of change electron's mind in terms of the decision they took.

Yeah, now that it is in a stable release I dont expect they would.
But hopefully a discussion can be started with them to coordinate changes in the future. As I said in an issue with them, my biggest problem is the lack of notice and documentation. While they did do a blog post which was published a couple of months before, and it was mentioned in the release notes, I expect very few users/maintainers to see those. Any documentation about this api has no mention of this limitation

[mhdawson]

@Julusian I do agree it would have been nice for some co-ordination with the Node-API team to have been done up front. The more implementers we have the more important that becomes. Where there specific people that you talked to on the electron side who might be good people to reach out to?

[Julusian]

All the discussion I have had with the elcecton team has been in an issue https://github.com/electron/electron/issues/35801

[mhdawson]

@Julusian thanks for the pointer to the issue/discussion.

[mhdawson]

This comment from https://github.com/electron/electron/issues/35801

I think I'd still prefer a stable API to detect the cage up-front rather than having the process crash. Failing that, we can add something explicit to the sharp API that allows someone developing for Electron 21+ to opt-in to copying the Buffer (and therefore opt-in to the performance and memory fragmentation cost).

Is worth discussing in the next Node-api team meeting.

[mhdawson]

Created this PR to do what we discussed in the last node-api team meeting - https://github.com/nodejs/node/pull/45181

[KevinEady]

We discussed this in the 11 Nov Node API meeting.

Since https://github.com/nodejs/node/pull/45181 has been merged, there is no additional work for the Node API team regarding this issue. We can close this issue @mhdawson .

[mhdawson]

@KevinEady at this point we just need the PRs to be backported to earlier release lines, but I'm ok with closing.