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

n-api: implement napi_run_script #15216

Closed

Conversation

gabrielschulhof
Copy link
Contributor

Fixes: nodejs/abi-stable-node#51

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

n-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 6, 2017
@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Sep 6, 2017
doc/api/n-api.md Outdated
```

- `[in] env`: The environment that the API is invoked under.
- `[in] script`: The string containing the script to execute.
Copy link
Member

Choose a reason for hiding this comment

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

Make it clear that this is in UTF-8? Or just take a string napi_value instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it'd be easier to take a napi_value instead.

doc/api/n-api.md Outdated
```

- `[in] env`: The environment that the API is invoked under.
- `[in] script`: The string containing the script to execute.
Copy link
Member

Choose a reason for hiding this comment

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

… encoded as UTF-8 and null-terminated?

NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &arg, NULL, NULL));

NAPI_CALL(env, napi_get_value_string_utf8(env, arg, NULL, 0, &string_size));
string = (char *)malloc(string_size);
Copy link
Member

Choose a reason for hiding this comment

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

don’t we technically need to free() this?

@@ -215,12 +216,35 @@ napi_value testAdjustExternalMemory(napi_env env, napi_callback_info info) {
return result;
}

napi_value testNapiRun(napi_env env, napi_callback_info info) {
size_t string_size;
char *string;
Copy link
Member

Choose a reason for hiding this comment

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

Pointer star goes with the type.

Copy link
Member

Choose a reason for hiding this comment

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

We’re inconsistent about that in this file, and I think this can pass as “idiomatic for C code”

NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &arg, NULL, NULL));

NAPI_CALL(env, napi_get_value_string_utf8(env, arg, NULL, 0, &string_size));
string = (char *)malloc(string_size);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

doc/api/n-api.md Outdated
- `[in] script`: The string containing the script to execute.
- `[out] result`: The value resulting from having executed the script.

[Script Execution]: #n_api_script_execution
Copy link
Member

Choose a reason for hiding this comment

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

Alphabetical order

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Sep 6, 2017

Switched script parameter to napi_value, made sure that the script ->IsString(), and added a test to make sure it throws if it isn't.

@gabrielschulhof gabrielschulhof force-pushed the napi-run-script branch 2 times, most recently from 07920bb to c7d03d4 Compare September 6, 2017 12:27
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

In the future we might want to have an equivalent for V8's Script or UnboundScript, but this LGTM and should suffice for now.

doc/api/n-api.md Outdated
@@ -3556,6 +3557,25 @@ NAPI_EXTERN napi_status napi_is_promise(napi_env env,
- `[out] is_promise`: Flag indicating whether `promise` is a native promise
object - that is, a promise object created by the underlying engine.

## Script execution

N-API allows you to execute a string containing JavaScript using the underlying
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid the use of you in the docs.

N-API allows the execution of a JavaScript string using the underlying JavaScript engine.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM once comment from @jasnell is addressed

@gabrielschulhof
Copy link
Contributor Author

@jasnell I have reworded the sentence.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

thank you!

@BridgeAR
Copy link
Member

BridgeAR commented Sep 9, 2017

@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Sep 9, 2017
Fixes: nodejs/abi-stable-node#51
PR-URL: nodejs#15216
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
@gabrielschulhof gabrielschulhof deleted the napi-run-script branch September 9, 2017 23:16
@gabrielschulhof
Copy link
Contributor Author

Landed in 61e9ba1.

MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
Fixes: nodejs/abi-stable-node#51
PR-URL: #15216
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
Fixes: nodejs/abi-stable-node#51
PR-URL: #15216
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
Fixes: nodejs/abi-stable-node#51
PR-URL: #15216
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
Fixes: nodejs/abi-stable-node#51
PR-URL: nodejs#15216
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
@digitalinfinity
Copy link
Contributor

@gabrielschulhof sorry, missed reviewing this PR. @MSLaguana and I were looking at it for implementation in node-chakracore and we had a question- if someone was debugging their script and a script executed from this API was on the stack- what is the "filename" they expect to see? Dynamic code or something equivalent? Should there be a way of modules identifying themselves to the debugger? Or should these frames be considered "library code" and hidden from the debugger?

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Sep 15, 2017 via email

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
Fixes: nodejs/abi-stable-node#51
PR-URL: nodejs#15216
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
Fixes: nodejs/abi-stable-node#51
Backport-PR-URL: #19447
PR-URL: #15216
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
tniessen added a commit to tniessen/node-addon-api that referenced this pull request Dec 1, 2019
This is a thin wrapper around napi_run_script.

Refs: nodejs/node#15216
tniessen added a commit to tniessen/node-addon-api that referenced this pull request Dec 1, 2019
This is a thin wrapper around napi_run_script.

Refs: nodejs/node#15216
tniessen added a commit to tniessen/node-addon-api that referenced this pull request Dec 1, 2019
This is a thin wrapper around napi_run_script.

Refs: nodejs/node#15216
tniessen added a commit to tniessen/node-addon-api that referenced this pull request Dec 2, 2019
This is a thin wrapper around napi_run_script.

Refs: nodejs/node#15216
tniessen added a commit to tniessen/node-addon-api that referenced this pull request Dec 2, 2019
This is a thin wrapper around napi_run_script.

Refs: nodejs/node#15216
gabrielschulhof pushed a commit to nodejs/node-addon-api that referenced this pull request Dec 14, 2019
This is a thin wrapper around napi_run_script.

Refs: nodejs/node#15216
PR-URL: #616
Reviewed-By: Nicola Del Gobbo <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Kevin Eady <[email protected]>
Reviewed-By: Kevin Eady <[email protected]>
gabrielschulhof pushed a commit to nodejs/node-addon-api that referenced this pull request Dec 14, 2019
This is a thin wrapper around napi_run_script.

Refs: nodejs/node#15216
PR-URL: #616
Reviewed-By: Nicola Del Gobbo <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Kevin Eady <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
This is a thin wrapper around napi_run_script.

Refs: nodejs/node#15216
PR-URL: nodejs/node-addon-api#616
Reviewed-By: Nicola Del Gobbo <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Kevin Eady <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
This is a thin wrapper around napi_run_script.

Refs: nodejs/node#15216
PR-URL: nodejs/node-addon-api#616
Reviewed-By: Nicola Del Gobbo <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Kevin Eady <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
This is a thin wrapper around napi_run_script.

Refs: nodejs/node#15216
PR-URL: nodejs/node-addon-api#616
Reviewed-By: Nicola Del Gobbo <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Kevin Eady <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
This is a thin wrapper around napi_run_script.

Refs: nodejs/node#15216
PR-URL: nodejs/node-addon-api#616
Reviewed-By: Nicola Del Gobbo <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Kevin Eady <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
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++. lib / src Issues and PRs related to general changes in the lib or src directory. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NODE_EXTERN napi_value napi_run_script(napi_env e, const char *script);