n-api: move cleanup_hook to version 4

[kfarnung]

Update N-API to version 4 and move the cleanup_hook API into it.

Continuation from #19962 (comment), I made the code changes but the viability is still pending.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@kfarnung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/271/pipeline

[mhdawson]

@kfarnung can you research how requested/landed functions for the hooks and then see if they can help us figure out who this might break?

[kfarnung]

@mhdawson These functions went into n-api in v10.2.0, so I don't think it's a good idea to move them into a new version. They haven't gone further downstream yet, but given how long they've been in v10.x, I don't think there's much we can do.

[kfarnung]

@addaleax Any thoughts on how to proceed. Since the functions are named exports any modules built using the original header would continue to link and run, but if the author went to build against a newer version of N-API they would need to #def NAPI_VERSION 4 to get the code building again.

[addaleax]

@kfarnung I don’t know, I don’t think introducing version guards after-the-fact was a great step in the first place tbh … I’m sorry I didn’t follow that when I was implementing it.

I don’t think much code is using these methods yet, but with the implementation of workers that might increase a lot, actually…

[kfarnung]

The initial design when adding the version guards was to roughly map the older versions and then call everything that existed version 3. One of the problems is that we didn't bump the N-API version when the cleanup hooks were added so technically N-API version 3 may or may not include the cleanup hooks.

One thought was to fix it now such that we bump the N-API version to 4 and move cleanup to that, but we could just take this as a life lesson and move on. There's a formal process for adding new N-API functionality being discussed, so hopefully that will help resolve these going forward.

Just to clarify, are you opposed to having the version guards all up or just moving this API forward into a newer version? My main concern which lead me to add them was that it's currently very hard to target a subset of the N-API surface without digging into the code to figure out when a particular API was added and what versions it was added to.

[addaleax]

@kfarnung I’m not opposed to anything in particular. I do think that putting these particular functions behind a version guard is a breaking change, and we generally don’t do that unless we have a really good reason to do so. But either way, I’ll defer the decision to those who are more actively involved in N-API work. :)

[kfarnung]

Thanks for the insights, that's what I figured you were saying.

@mhdawson I agree with @addaleax that since this already shipped it would be considered a breaking change. I think it's probably best to just leave it in version 3 (which was the active version at the time we shipped them) and just formalize the process going forward.

[mhdawson]

The main concern is that they are not in 6.x or 8.x which claim support for version 3. So if you compile with 10.x requesting version 3 to be compatible with 6.x and 8.x and use these functions then people will run into problems in that they don't exist in 6.x and 8.x.

There is no good choice, but maybe the best is to simply backport them to 6.x and 8.x. Since we tell people its "forward" compatibility most people should really be compiling on 6.x to get support for LTS versions anyway so it may not end up being a big deal, backporting would close the hole though.

[jasnell]

What's the status on this one?

[kfarnung]

@gabrielschulhof What do you think the viability of backporting these functions is?

[gabrielschulhof]

cleanup hooks will land on v6.x as version 3, AFAIK.

[kfarnung]

Thanks @gabrielschulhof, sounds like I can just close this PR and the issue will resolve once the backports are complete.