Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experimental node_api_nogc_env is not stripped #51

Closed
legendecas opened this issue Oct 4, 2024 · 4 comments · Fixed by #52
Closed

Experimental node_api_nogc_env is not stripped #51

legendecas opened this issue Oct 4, 2024 · 4 comments · Fixed by #52
Assignees

Comments

@legendecas
Copy link
Member

legendecas commented Oct 4, 2024

Ref:

#if !defined(NAPI_EXPERIMENTAL) || \
(defined(NAPI_EXPERIMENTAL) && \
(defined(NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT) || \
defined(NODE_API_EXPERIMENTAL_BASIC_ENV_OPT_OUT)))
typedef struct napi_env__* node_api_nogc_env;
#else
typedef const struct napi_env__* node_api_nogc_env;
#endif

All other NAPI_EXPERIMENTAL apis were stripped, but node_api_nogc_env and node_api_nogc_finalize were not.

If NAPI_EXPERIMENTAL is defined, node_api_post_finalizer is missing for node_api_nogc_finalize callbacks.

@KevinEady
Copy link
Contributor

KevinEady commented Oct 11, 2024

Root problem: the current experimental stripping can only support #if(n)def NAPI_EXPERIMENTAL, and so the conditional here is a bit too complex:

if (matches = line.match(/^\s*#if(n)?def\s+([A-Za-z_][A-Za-z0-9_]*)/)) {

@KevinEady
Copy link
Contributor

We discussed in the 11 Oct Node-API meeting a possible solution of modifying the Node.js header files such that it would be easier for the tool to correctly remove experimental features.

Thinking about it some more, I am not a big fan of this approach. I don't think we should "dumb down" the header files just because the tool isn't smart enough.

I am looking into making the tool more robust and handling #if as well.

@legendecas
Copy link
Member Author

Just a idea: do we really need to strip experimental APIs?

If the experimental APIs were not stripped, it could be tested in nodejs/node-addon-api#1585 as well.

@NickNaso
Copy link
Member

What I remember about the decision to strip experimental APIs is that we would like to avoid developers use APIs that could be removed and then breaks their build.

@github-project-automation github-project-automation bot moved this from Has PR to Done in Node-API Team Project Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants