[v14.x] node-api: handle no support for external buffers#45616
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Refs: electron/electron#35801 Refs: nodejs/abi-stable-node#441 Electron recently dropped support for external buffers. Provide a way for addon authors to: - hide the methods to create external buffers so they can avoid using them if they want the broadest compatibility. - call the methods that create external buffers at runtime to check if external buffers are supported and either use them or not based on the return code. Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#45181 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
nodejs-github-bot commented Nov 24, 2022
Review requested:
|
nodejs-github-bot added c++ 
@legendecas if you could review this backport that would be great. |
richardlau changed the title 
github-actionsbot removed the request-ci nodejs-github-bot commented Nov 24, 2022
nodejs-github-bot commented Nov 25, 2022
| NAPI_PREAMBLE(env); | ||
| CHECK_ARG(env, result); | ||
| #if defined(V8_ENABLE_SANDBOX) |
There was a problem hiding this comment.
I don't find this definition on v14.x. Maybe we can simply remove this change?
There was a problem hiding this comment.
That could make sense, I think we wanted the backport so that people could compile on the oldest LTS and use the value for the error code.
They question is whether it's better to keep it closer to the original PR even if the define will never be set or to remove it?
There was a problem hiding this comment.
@richardlau do you have an opinion from the Releaser perspective? I'm happy either way. We can remove the define or leave it in to make it a "backport" versus a "backport" with modifications whichever is easier/preferrable for getting it landed.
There was a problem hiding this comment.
@mhdawson No preference either way. I think it's very unlikely V8_ENABLE_SANDBOX would be introduced to Node.js 14 if it isn't already there given that Node.js 14 is in maintenance and will go End-of-Life in April.
cc @nodejs/lts
There was a problem hiding this comment.
@legendecas since @richardlau figures there is no preference from the Releasers perspective I don't don't feel strongly either. I'm happy to leave as it as I don't think it hurts anything but also happy to remove that i you think that's best.
There was a problem hiding this comment.
I'm fine with both ways. Thanks for the clarification.
| NAPI_PREAMBLE(env); | ||
| CHECK_ARG(env, result); | ||
| #if defined(V8_ENABLE_SANDBOX) |
There was a problem hiding this comment.
I'm fine with both ways. Thanks for the clarification.
mhdawson commented Nov 29, 2022
@richardlau I'll leave as is given the discussion. |
Refs: electron/electron#35801 Refs: nodejs/abi-stable-node#441 Electron recently dropped support for external buffers. Provide a way for addon authors to: - hide the methods to create external buffers so they can avoid using them if they want the broadest compatibility. - call the methods that create external buffers at runtime to check if external buffers are supported and either use them or not based on the return code. Signed-off-by: Michael Dawson <[email protected]> PR-URL: #45181 Backport-PR-URL: #45616 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
richardlau commented Dec 7, 2022
Landed in 2dbeb88. |
4 participants
Refs: electron/electron#35801 Refs: nodejs/abi-stable-node#441
Electron recently dropped support for external
buffers. Provide a way for addon authors to:
Signed-off-by: Michael Dawson [email protected]
PR-URL: #45181
Reviewed-By: Chengzhong Wu [email protected]
Reviewed-By: Minwoo Jung [email protected]