n-api: implement napi_run_script by gabrielschulhof · Pull Request #15216 · nodejs/node

gabrielschulhof · 2017-09-06T10:53:34Z

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

TimothyGu · 2017-09-06T11:21:12Z

doc/api/n-api.md

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

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

addaleax · 2017-09-06T11:17:53Z

doc/api/n-api.md

… encoded as UTF-8 and null-terminated?

addaleax · 2017-09-06T11:21:27Z

test/addons-napi/test_general/test_general.c

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

TimothyGu · 2017-09-06T11:22:43Z

test/addons-napi/test_general/test_general.c

Pointer star goes with the type.

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

TimothyGu · 2017-09-06T11:22:48Z

test/addons-napi/test_general/test_general.c

TimothyGu · 2017-09-06T11:23:10Z

doc/api/n-api.md

Alphabetical order

gabrielschulhof · 2017-09-06T12:21:43Z

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.

TimothyGu

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

jasnell · 2017-09-06T20:55:03Z

doc/api/n-api.md

Please avoid the use of you in the docs.

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

mhdawson

LGTM once comment from @jasnell is addressed

Fixes: nodejs/abi-stable-node#51 PR-URL: nodejs#15216

gabrielschulhof · 2017-09-06T22:10:04Z

@jasnell I have reworded the sentence.

jasnell

thank you!

BridgeAR · 2017-09-09T19:40:33Z

CI https://ci.nodejs.org/job/node-test-pull-request/10009/

gabrielschulhof · 2017-09-09T22:00:23Z

SmartOS re-run: https://ci.nodejs.org/job/node-test-commit-smartos/11353/

gabrielschulhof · 2017-09-09T23:05:22Z

ARM re-run: https://ci.nodejs.org/job/node-test-commit-arm/11989/

Fixes: nodejs/abi-stable-node#51 PR-URL: nodejs#15216 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com Reviewed-By: James M Snell <jasnell.gmail.com> Reviewed-By: Michael Dawson <mhdawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

gabrielschulhof · 2017-09-09T23:16:40Z

Landed in 61e9ba1.

Fixes: nodejs/abi-stable-node#51 PR-URL: #15216 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com Reviewed-By: James M Snell <jasnell.gmail.com> Reviewed-By: Michael Dawson <mhdawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

Fixes: nodejs/abi-stable-node#51 PR-URL: nodejs#15216 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com Reviewed-By: James M Snell <jasnell.gmail.com> Reviewed-By: Michael Dawson <mhdawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

digitalinfinity · 2017-09-14T23:48:26Z

@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 · 2017-09-15T09:53:46Z

@digitalinfinity maybe we should treat it like an eval. Chrome developer tools opens a new tab and calls it "VM<number>" where number is probably some internal counter or something like that. It paints the tab a different colour. In fact, I ran `node --inspect-brk test/addons-napi/test_general/testNapiRun.js` and when I stepped into `addon.testNapiRun()` it behaved exactly like `eval()`.

…

On Fri, Sep 15, 2017 at 2:49 AM, Hitesh Kanwathirtha < ***@***.***> wrote: @gabrielschulhof <https://github.com/gabrielschulhof> sorry, missed reviewing this PR. @MSLaguana <https://github.com/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? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15216 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7k0etVgsKC-XXzJmDMQLQoyJOln7O1ks5sibt6gaJpZM4POMQh> .

Fixes: nodejs/abi-stable-node#51 PR-URL: nodejs#15216 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com Reviewed-By: James M Snell <jasnell.gmail.com> Reviewed-By: Michael Dawson <mhdawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

Fixes: nodejs/abi-stable-node#51 Backport-PR-URL: #19447 PR-URL: #15216 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com Reviewed-By: James M Snell <jasnell.gmail.com> Reviewed-By: Michael Dawson <mhdawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216

This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216 PR-URL: #616 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com> Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com>

This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216 PR-URL: #616 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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 <nicoladelgobbo@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Kevin Eady <kevin.c.eady@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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 added the node-api Issues and PRs related to the Node-API. label Sep 6, 2017

TimothyGu reviewed Sep 6, 2017

View reviewed changes

addaleax approved these changes Sep 6, 2017

View reviewed changes

TimothyGu reviewed Sep 6, 2017

View reviewed changes

gabrielschulhof force-pushed the napi-run-script branch from 679ade8 to 402d5be Compare September 6, 2017 12:20

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

cjihrig approved these changes Sep 6, 2017

View reviewed changes

benjamingr approved these changes Sep 6, 2017

View reviewed changes

TimothyGu approved these changes Sep 6, 2017

View reviewed changes

jasnell requested changes Sep 6, 2017

View reviewed changes

mhdawson approved these changes Sep 6, 2017

View reviewed changes

n-api: implement napi_run_script

ca0d064

Fixes: nodejs/abi-stable-node#51 PR-URL: nodejs#15216

gabrielschulhof force-pushed the napi-run-script branch from c7d03d4 to ca0d064 Compare September 6, 2017 22:09

jasnell approved these changes Sep 6, 2017

View reviewed changes

gabrielschulhof closed this Sep 9, 2017

gabrielschulhof deleted the napi-run-script branch September 9, 2017 23:16

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the dont-land-on-v6.x label Sep 20, 2017

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

tniessen added a commit to tniessen/node-addon-api that referenced this pull request Dec 1, 2019

Add Env::RunScript

2a5cc1c

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

Add Env::RunScript

f196271

This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216

tniessen mentioned this pull request Dec 1, 2019

Add Env::RunScript nodejs/node-addon-api#616

Closed

tniessen added a commit to tniessen/node-addon-api that referenced this pull request Dec 1, 2019

Add Env::RunScript

92dc802

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

Add Env::RunScript

25740d1

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

Add Env::RunScript

c0b2070

This is a thin wrapper around napi_run_script. Refs: nodejs/node#15216

Uh oh!

Conversation

gabrielschulhof commented Sep 6, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Sep 6, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 9, 2017

Uh oh!

gabrielschulhof commented Sep 9, 2017

Uh oh!

gabrielschulhof commented Sep 9, 2017

Uh oh!

gabrielschulhof commented Sep 9, 2017

Uh oh!

digitalinfinity commented Sep 14, 2017

Uh oh!

gabrielschulhof commented Sep 15, 2017 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

gabrielschulhof commented Sep 6, 2017 •

edited

Loading