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

[node-api]: New variant napi_set_named_property that treats '\0' as values. #52979

Closed
hyf0 opened this issue May 14, 2024 · 4 comments
Closed
Labels
feature request Issues that request new features to be added to Node.js. node-api Issues and PRs related to the Node-API.

Comments

@hyf0
Copy link

hyf0 commented May 14, 2024

What is the problem this feature will solve?

I'm developing rolldown, rust version of rollup. Rollup use \0 as the virtual module name convention.

If your plugin uses 'virtual modules' (e.g. for helper functions), prefix the module ID with \0. This prevents other plugins from trying to process it. link

Rolldown follows this convention and got problems while using napi_set_named_property. napi_set_named_property considers \0 as the terminator rather than values. This makes passing "\0mvirtual-module" to Node using napi_set_named_property become "".

Related issues:

What is the feature you are proposing to solve the problem?

So I propose a new variant napi_set_named_property, maybe call ``napi_set_named_property_len`, which will receive string and the length. This way, it could treat '\0' as values.

For alignment, If this request is accepted, I guess napi_get_named_property need a new variant too.

What alternatives have you considered?

I'm using workaround said in rolldown/rolldown#1115 (comment). Create JsString first, then using napi_set_property. But this cause extra napi calls and it's really not good for a bundler that address speed.

@hyf0 hyf0 added the feature request Issues that request new features to be added to Node.js. label May 14, 2024
@legendecas
Copy link
Member

If the property keys are definite, creating property keys with node_api_create_property_key_utf16 beforehand and reuse the created keys in napi_set_property could helps.

@legendecas legendecas added the node-api Issues and PRs related to the Node-API. label May 14, 2024
@hyf0
Copy link
Author

hyf0 commented May 14, 2024

If the property keys are definite, creating property keys with node_api_create_property_key_utf16 beforehand and reuse the created keys in napi_set_property could helps.

Thanks for the solution. I'm using the workaround like it, but still there is an extra napi call of node_api_create_property_key_utf16.

@mertcanaltin
Copy link
Member

I tried to make a quick improvement for this place @hyf0 @legendecas

@KevinEady
Copy link
Contributor

Closing issue, as #52984 has landed, adding node_api_create_property_key_utf8 and node_api_create_property_key_latin1. These functions (as well as the existing node_api_create_property_key_utf16) give the developer the ability to pass in a size_t length of the string and bypass using \0 as terminator for deciding string length.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. node-api Issues and PRs related to the Node-API.
Projects
Archived in project
Development

No branches or pull requests

4 participants