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

Optional Value Boxes #690

Closed
legendecas opened this issue Mar 26, 2020 · 7 comments · Fixed by #927
Closed

Optional Value Boxes #690

legendecas opened this issue Mar 26, 2020 · 7 comments · Fixed by #927
Assignees

Comments

@legendecas
Copy link
Member

Napi::Value can be empty. Many of N-API will return napi_invalid_arg on empty Napi::Value as a parameter (e.g. napi_get_property). We can introduce a Maybe value box to node-addon-api to early detect these conditions and provide a clean way to indicate whether a parameter Napi::Value is optional.

@legendecas legendecas self-assigned this Mar 26, 2020
@legendecas
Copy link
Member Author

I'd believe this can be a better solution for #544.

@devsnek
Copy link
Member

devsnek commented Mar 26, 2020

Napi::Value already provides IsEmpty(). Maybe<> in the V8 API is used to represent things that throw, not things that are optional.

@legendecas
Copy link
Member Author

legendecas commented Mar 27, 2020

Yeah, there is already an IsEmpty on Napi::Value. Still, there is no ability to provide a strong type guarantee that a Napi::SomeType is absolutely not empty, i.e. compiler will complain about the code on build time.

If Maybe is not an appropriate name for the situation, I'm ok with another name that fits.

@legendecas legendecas changed the title Maybe Value Boxes Optional Value Boxes Mar 27, 2020
@devsnek
Copy link
Member

devsnek commented Mar 27, 2020

Adding Maybe wouldn't fix the issue that there's no way to guarantee a value is not empty because the Value in the Maybe can still be empty. I do agree it would be cool if it were possible but that might be outside the bounds of the language we've chosen to work with here.

@legendecas
Copy link
Member Author

legendecas commented Apr 1, 2020

The idea is to distinguish empty-able parameters and return values at build time.

In the example of #544, originally we have to use a trick to achieve what we expect:

Function callback; // this one is empty since it's not initialized with any napi_value
auto tsfn = ThreadSafeFunction::New(env, callback, resource_name, 1, 1);

With optional boxes, we can make Napi::Value not constructible in the way above, and use Optional::Empty instead:

auto tsfn = ThreadSafeFunction::New(env, Napi::Optional::Empty(), resource_name, 1, 1);

If we do have a callback value:

Napi::Function callback = Napi::Function::New(env, some_callback);
auto tsfn = ThreadSafeFunction::New(env, Napi::Optional::Just(callback), resource_name, 1, 1);

Or make it implicit:

Napi::Function callback = Napi::Function::New(env, some_callback);
auto tsfn = ThreadSafeFunction::New(env, callback, resource_name, 1, 1);

In this way, we can make our API stronger than just empty-able Napi::Value, with which compiler doesn't complain if it's empty where it should not be empty.

Optional is available in stl since C++ 17, so maybe we can use stl optional directly.

@github-actions
Copy link
Contributor

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2021

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

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

Successfully merging a pull request may close this issue.

2 participants