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

Add Env::RunScript #616

Open
wants to merge 1 commit into
base: master
from
Open

Add Env::RunScript #616

wants to merge 1 commit into from

Conversation

@tniessen
Copy link
Member

tniessen commented Dec 1, 2019

This is a thin wrapper around napi_run_script. I don't feel strongly about this addition, but if we do want to add it, it might be worth considering using Napi::String instead of Napi::Value for the script parameter. (I wasn't sure which one to pick.)

Refs: nodejs/node#15216

@tniessen tniessen force-pushed the tniessen:add-env-run-script branch from f196271 to 92dc802 Dec 1, 2019
@tniessen tniessen added the enhancement label Dec 1, 2019
Copy link
Member

NickNaso left a comment

Just some suggestions

doc/env.md Outdated Show resolved Hide resolved
doc/env.md Outdated Show resolved Hide resolved
@tniessen tniessen force-pushed the tniessen:add-env-run-script branch from 92dc802 to 25740d1 Dec 2, 2019
Copy link
Member

NickNaso left a comment

LGTM

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Dec 2, 2019

I think this should definitely take a Napi::String.

This is a thin wrapper around napi_run_script.

Refs: nodejs/node#15216
@tniessen tniessen force-pushed the tniessen:add-env-run-script branch from 25740d1 to c0b2070 Dec 2, 2019
@tniessen

This comment has been minimized.

Copy link
Member Author

tniessen commented Dec 2, 2019

@devsnek Done.

@KevinEady

This comment has been minimized.

Copy link
Contributor

KevinEady commented Dec 2, 2019

Sorry to jump in late, buuuut ... I think it should take an napi_value because the actual napi_run_script method takes an napi_value, and in order to avoid semantic differences between underlying napi and our C++ wrapper (eg. #594), I think we should try to stick close to napi as possible. Plus, the current napi implementation already handles non-String values as errors -- I don't think it's something that node-addon-api needs to handle separately.

I take back everything above. The added enforcement of type-checking is a 👍 provided by node-addon-api. Sorry for flip-flops!

@devsnek
devsnek approved these changes Dec 3, 2019
Copy link
Member

mhdawson left a comment

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.