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: add napi_get_symbol_to_string_tag() #41370

Conversation

RaisinTen
Copy link
Contributor

Fixes: #41358

Signed-off-by: Darshan Sen darshan.sen@postman.com

Fixes: nodejs#41358

Signed-off-by: Darshan Sen <darshan.sen@postman.com>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jan 1, 2022
@nodejs-github-bot
Copy link
Collaborator

@motla
Copy link

motla commented Jan 1, 2022

Hi @RaisinTen! Great work, and especially fast for a holiday season, so thank you very much!

This PR is perfect for me, but while you made this, I realized it would be nice to add the other "well-known Symbols" specified by ECMAScript as well, as it may be requested in the future.

https://tc39.es/ecma262/#sec-well-known-symbols

  • Symbol.asyncIterator
  • Symbol.hasInstance
  • Symbol.isConcatSpreadable
  • Symbol.iterator
  • Symbol.match
  • Symbol.matchAll
  • Symbol.replace
  • Symbol.search
  • Symbol.species
  • Symbol.split
  • Symbol.toPrimitive
  • Symbol.toStringTag
  • Symbol.unscopables

So instead of napi_get_symbol_to_string_tag, my proposal is under the "Functions to get global instances" section I suggest a generic:

napi_status napi_get_symbol(napi_env env, napi_symbol_name name, napi_value* result)

And under the "Structures" section:

typedef enum {
  napi_symbol_async_iterator,
  napi_symbol_has_instance,
  napi_symbol_is_concat_spreadable,
  napi_symbol_iterator,
  napi_symbol_match,
  napi_symbol_match_all,
  napi_symbol_replace,
  napi_symbol_search,
  napi_symbol_species,
  napi_symbol_split,
  napi_symbol_to_primitive,
  napi_symbol_to_string_tag,
  napi_symbol_unscopables
} napi_symbol_name;

This is a minor modification: just rename the function and add a switch/case inside. It will be much cleaner for the API, what do you think?
(I'm not sure about the typedef naming above, it may need a review...)

@addaleax addaleax added the node-api Issues and PRs related to the Node-API. label Jan 1, 2022
@addaleax addaleax changed the title src: add napi_get_symbol_to_string_tag() node-api: add napi_get_symbol_to_string_tag() Jan 1, 2022
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What’s the problem with getting these symbols of the global Symbol variable? Obviously we could do what this PR suggests, but that seems like a slippery slope to adding getters to the Node-API interface for every single JS builtin.

@motla
Copy link

motla commented Jan 1, 2022

What’s the problem with getting these symbols of the global Symbol variable?

Can we access the global Symbol variable in C/C++ with Node-API? What do you suggest?

(OP is here: #41358)

@addaleax
Copy link
Member

addaleax commented Jan 1, 2022

Can we access the global Symbol variable in C/C++ with Node-API? What do you suggest?

Yes, and that is would I would indeed suggest :)

@RaisinTen
Copy link
Contributor Author

@addaleax

What’s the problem with getting these symbols of the global Symbol variable?

  • Getting the symbol from the global object would require using strings (prone to typos), which doesn't allow them to be programmatically accessible.
  • As [Node-API] Symbol.toStringTag support #41358 (comment) shows, it requires the user to write more lines of code, so having this API would be convenient to have, at least when we want to call this from C.
  • It also requires more API calls to get the symbol from the global object than just calling the API exposed in this PR, which makes this implementation faster.

Obviously we could do what this PR suggests, but that seems like a slippery slope to adding getters to the Node-API interface for every single JS builtin.

How is that wrong? We already have APIs like

  • napi_throw_error
  • napi_throw_type_error
  • napi_throw_range_error
  • node_api_throw_syntax_error

when just napi_throw() would be enough.

Also constructors like

  • napi_create_error
  • napi_create_type_error
  • napi_create_range_error
  • node_api_create_syntax_error

are not needed if we can just call napi_new_instance() on the constructors that can be accessed on the global object.

@addaleax
Copy link
Member

addaleax commented Jan 3, 2022

@addaleax

What’s the problem with getting these symbols of the global Symbol variable?

* Getting the symbol from the global object would require using strings (prone to typos), which doesn't allow them to be programmatically accessible.

Well … that’s also true for JS code which references Symbol.toStringTag. I don’t think this is a strong argument, tbh.

* As [[Node-API] Symbol.toStringTag support #41358 (comment)](https://github.com/nodejs/node/issues/41358#issuecomment-1003698071) shows, it requires the user to write more lines of code, so having this API would be convenient to have, at least when we want to call this from C.

Well – If you want convenience, you probably won’t use C to begin with. And as shown in that thread, in the C++ convenience wrapper API, it’s a one-liner.

That being said, of course it’s nice to have a helper API for this in C as well – but that doesn’t mean it has to be in the core Node-API. Somebody (e.g. you) could just publish a header library to npm which provides this. (I’ve done that in the past for other things, and it works just fine, e.g. setimmediate-napi).

* It also requires more API calls to get the symbol from the global object than just calling the API exposed in this PR, which makes this implementation faster.

That’s true, but I wouldn’t consider performance a concern. If it is, people can cache the value in their addons.

Obviously we could do what this PR suggests, but that seems like a slippery slope to adding getters to the Node-API interface for every single JS builtin.

How is that wrong? We already have APIs like

* napi_throw_error

* napi_throw_type_error

* napi_throw_range_error

* node_api_throw_syntax_error

when just napi_throw() would be enough.

Also constructors like

* napi_create_error

* napi_create_type_error

* napi_create_range_error

* node_api_create_syntax_error

are not needed if we can just call napi_new_instance() on the constructors that can be accessed on the global object.

Great point! I should have argued against adding these specific methods when Node-API was being introduced. (That being said, I assume part of the motivation here was matching what the V8 API provides out of the box.)

@mhdawson
Copy link
Member

mhdawson commented Jan 4, 2022

Looks like #41358 was closed with an alternate solution. Should this be closed?

@motla
Copy link

motla commented Jan 4, 2022

Concerning my issue #41358, I'm fine with the solution proposed by @addaleax.
I feel really sorry for @RaisinTen who was very reactive on the issue and did exactly what I asked for. It would just have been cool to have some strategy discussion before working on the new feature because my proposal was not the best due to my low experience with the API and not knowing Symbol was accessible via global.
As there are many other well-known symbols like toStringTag I realized it makes no sense to have a separate getter function for each of these Symbols.
However a napi_get_symbol sugar based on this PR and like described above (#41370 (comment)) could be useful for users. Whether to implement it or not is to be discussed with your team, but it can be for another PR.

@RaisinTen
Copy link
Contributor Author

Looks like #41358 was closed with an alternate solution. Should this be closed?

@mhdawson the OP might still benefit from what's exposed from this PR / the napi_get_symbol() approach described in the above comment. I was wondering if we should clarify what kinds of APIs we want to include in Node-API before closing/going ahead with this PR because I got some approvals in #41329 which implements something similar. #41329 (comment) explains what's still left to be clarified.

@mhdawson
Copy link
Member

mhdawson commented Jan 7, 2022

@RaisinTen this is the guidance we have - https://github.com/nodejs/node/blob/master/doc/guides/adding-new-napi-api.md

As mentioned by @addaleax it still does often come down to a grey line. I'm ok with #41329 landing as it seemed more limited in scope, while this one seemed to grow the scope beyond the spirit of Must be a necessary API and not a nice to have. Convenience APIs belong in node-addon-api. (from adding-new-api-napi-api.md).

@RaisinTen
Copy link
Contributor Author

Thanks for the clarification, closing :)

@RaisinTen RaisinTen closed this Jan 8, 2022
@RaisinTen RaisinTen deleted the src/add-napi_get_symbol_to_string_tag branch January 8, 2022 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Node-API] Symbol.toStringTag support
5 participants