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

Discussion: Naming of envs #1527

Closed
gabrielschulhof opened this issue Jun 29, 2024 · 6 comments
Closed

Discussion: Naming of envs #1527

gabrielschulhof opened this issue Jun 29, 2024 · 6 comments

Comments

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Jun 29, 2024

Following up on our discussion during the meeting, let's jot down the possible names we'd like to use instead of env and nogc_env. Here are the scenarios I can think of so far:

napi_status napi_get_instance_data(_____ env, void** data); // currently node_api_nogc_env
void my_sync_finalizer(_____ env, void* data, void* hint); // currently node_api_nogc_env
void my_regular_finalizer(_____ env, void* data, void* hint); // currently napi_env
typedef void (*_____)(_____ env, void* data, void* hint); // currently napi_finalizer
typedef void (*_____)(_____ env, void* data, void* hint); // currently node_api_nogc_finalizer
napi_status napi_create_object(_____ env, napi_value* result); // currently napi_env

Inspired my Michael's emphasis that we're dealing with a base class - subclass relationship, I was thinking,

napi_status napi_get_instance_data(node_api_basic_env env, void** data);
void my_sync_finalizer(node_api_basic_env env, void* data, void* hint);
void my_regular_finalizer(napi_env env, void* data, void* hint);
typedef void (*napi_finalizer)(napi_env env, void* data, void* hint);
typedef void (*node_api_basic_finalizer)(node_api_basic_env, void* data, void* hint);
napi_status napi_create_object(napi_env env, napi_value* result);

since basic conveys the idea that fewer things can be done when you only have a "basic" environment, and since folks might be familiar with the base class/subclass relationship, whereby if a function accepts a base class then one can pass either, whereas if the function accepts a subclass and one only has a base class handy, then the function is off-limits.

We also need not necessarily elaborate on why an environment is basic. The fact that the context of being in a sync finalizer and therefore unable to manipulate engine heap is the reason why some APIs are off-limits is something we can document, but not absolutely necessarily.

WDYT?

@gabrielschulhof
Copy link
Contributor Author

Re #1514

@KevinEady
Copy link
Contributor

@gabrielschulhof ,

I really like the qualifier of "basic" for the env.

... basic conveys the idea that fewer things can be done when you only have a "basic" environment ...

Makes sense to me!

... since folks might be familiar with the base class/subclass relationship ...

And this paradigm goes well with the work we'll do in node-addon-api to incorporate the sync finalizers.

@KevinEady
Copy link
Contributor

@gabrielschulhof , regarding finalizers:

void my_sync_finalizer(node_api_basic_env env, void* data, void* hint);
void my_regular_finalizer(napi_env env, void* data, void* hint);

eg. for documentation, would you say that a finalizer can be categorized as either a "synchronous" finalizer or a "regular" finalizer? Perhaps it's better for "synchronous" or "asynchronous"?

@mhdawson
Copy link
Member

mhdawson commented Jul 5, 2024

node_api_basic_env sounds good to me as well.

@KevinEady
Copy link
Contributor

In the 5 July Node-API meeting, we settled on the finalizer accepting a node_api_basic_env being called a "basic finalizer" like in the original post:

typedef void (*node_api_basic_finalizer)(node_api_basic_env, void* data, void* hint);

We don't have any characterization of the existing napi_finalizer type. I could see it being called an "extended finalizer" if we ever need to have a distinction, since:

  • "extended" implies more behavior than "basic"
  • the "extended finalizer" takes as first argument a Napi::Env which is an extension of Napi::BasicEnv

For now, it may be okay to settle on eg. "A finalizer is a basic finalizer if <conditions>."

PTAL at https://github.com/KevinEady/node-addon-api/blob/add-nogc-types/doc/finalization.md for WIP documentation attached to #1514

@mhdawson
Copy link
Member

Agreement reached, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants