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

node-api: add status napi_cannot_run_js #47986

Closed

Conversation

gabrielschulhof
Copy link
Contributor

Add the new status in order to distinguish a state wherein an exception is pending from one wherein the engine is unable to execute JS. We take advantage of the new runtime add-on version reporting in order to remain forward compatible with add-ons that do not expect the new status code.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 13, 2023

Review requested:

  • @nodejs/gyp
  • @nodejs/node-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. needs-ci PRs that need a full CI run. labels May 13, 2023
@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label May 13, 2023
Add the new status in order to distinguish a state wherein an exception
is pending from one wherein the engine is unable to execute JS. We take
advantage of the new runtime add-on version reporting in order to remain
forward compatible with add-ons that do not expect the new status code.
@tniessen tniessen changed the title node-api: Add status napi_cannot_run_js node-api: add status napi_cannot_run_js May 13, 2023
doc/api/n-api.md Outdated Show resolved Hide resolved
test/node-api/test_cannot_run_js/test.js Outdated Show resolved Hide resolved
test/node-api/test_cannot_run_js/binding.gyp Outdated Show resolved Hide resolved
test/node-api/test_cannot_run_js/test_cannot_run_js.c Outdated Show resolved Hide resolved
@gabrielschulhof
Copy link
Contributor Author

@legendecas I have addressed your review comments. Please take another look!

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM % nits.

test/js-native-api/test_cannot_run_js/test.js Show resolved Hide resolved
@gabrielschulhof gabrielschulhof removed the needs-ci PRs that need a full CI run. label May 19, 2023
@nodejs-github-bot
Copy link
Collaborator

Some platforms do not seem to support target_defaults.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

gabrielschulhof added a commit that referenced this pull request May 20, 2023
Add the new status in order to distinguish a state wherein an exception
is pending from one wherein the engine is unable to execute JS. We take
advantage of the new runtime add-on version reporting in order to remain
forward compatible with add-ons that do not expect the new status code.

PR-URL: #47986
Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
fasenderos pushed a commit to fasenderos/node that referenced this pull request May 22, 2023
Add the new status in order to distinguish a state wherein an exception
is pending from one wherein the engine is unable to execute JS. We take
advantage of the new runtime add-on version reporting in order to remain
forward compatible with add-ons that do not expect the new status code.

PR-URL: nodejs#47986
Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
fasenderos pushed a commit to fasenderos/node that referenced this pull request May 22, 2023
Add the new status in order to distinguish a state wherein an exception
is pending from one wherein the engine is unable to execute JS. We take
advantage of the new runtime add-on version reporting in order to remain
forward compatible with add-ons that do not expect the new status code.

PR-URL: nodejs#47986
Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
kvakil added a commit to kvakil/node that referenced this pull request May 25, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the nodejs#1 failing JS test in the most recent reliability
report. Mark it as flaky.

Fixes: nodejs#48180
Refs: nodejs#47986
Refs: nodejs/reliability#576
kvakil added a commit to kvakil/node that referenced this pull request May 25, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the most common failing JS test in the most recent
reliability report. Mark it as flaky.

Fixes: nodejs#48180
Refs: nodejs#47986
Refs: nodejs/reliability#576
nodejs-github-bot pushed a commit that referenced this pull request May 27, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the most common failing JS test in the most recent
reliability report. Mark it as flaky.

Fixes: #48180
Refs: #47986
Refs: nodejs/reliability#576
PR-URL: #48181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request May 30, 2023
Add the new status in order to distinguish a state wherein an exception
is pending from one wherein the engine is unable to execute JS. We take
advantage of the new runtime add-on version reporting in order to remain
forward compatible with add-ons that do not expect the new status code.

PR-URL: #47986
Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
targos pushed a commit that referenced this pull request May 30, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the most common failing JS test in the most recent
reliability report. Mark it as flaky.

Fixes: #48180
Refs: #47986
Refs: nodejs/reliability#576
PR-URL: #48181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gabrielschulhof gabrielschulhof restored the cannot-call-into-js-status branch June 3, 2023 18:10
@targos targos mentioned this pull request Jun 4, 2023
@gabrielschulhof gabrielschulhof deleted the cannot-call-into-js-status branch June 5, 2023 02:21
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
Add the new status in order to distinguish a state wherein an exception
is pending from one wherein the engine is unable to execute JS. We take
advantage of the new runtime add-on version reporting in order to remain
forward compatible with add-ons that do not expect the new status code.

PR-URL: #47986
Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the most common failing JS test in the most recent
reliability report. Mark it as flaky.

Fixes: #48180
Refs: #47986
Refs: nodejs/reliability#576
PR-URL: #48181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
Add the new status in order to distinguish a state wherein an exception
is pending from one wherein the engine is unable to execute JS. We take
advantage of the new runtime add-on version reporting in order to remain
forward compatible with add-ons that do not expect the new status code.

PR-URL: nodejs#47986
Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the most common failing JS test in the most recent
reliability report. Mark it as flaky.

Fixes: nodejs#48180
Refs: nodejs#47986
Refs: nodejs/reliability#576
PR-URL: nodejs#48181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the most common failing JS test in the most recent
reliability report. Mark it as flaky.

Fixes: nodejs#48180
Refs: nodejs#47986
Refs: nodejs/reliability#576
PR-URL: nodejs#48181
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
This test has been failing occasionally since it was introduced ~5 days
ago. It was the most common failing JS test in the most recent
reliability report. Mark it as flaky.

Fixes: nodejs#48180
Refs: nodejs#47986
Refs: nodejs/reliability#576
PR-URL: nodejs#48181
Reviewed-By: Chengzhong Wu <[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.

3 participants