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.

Sign up for GitHub

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

Jump to bottom

src,doc: add SyntaxError napi support #40736

Closed
wants to merge 16 commits into from
Closed

src,doc: add SyntaxError napi support #40736

wants to merge 16 commits into from

Conversation

idan-at
Copy link
Contributor

@idan-at idan-at commented Nov 6, 2021

Add napi_create_syntax_error and napi_throw_syntax_error.

Fixes: nodejs/node-addon-api#1099

src,doc: add SyntaxError napi support
bdf8809 
Add `napi_create_syntax_error` and `napi_throw_syntax_error`.

Fixes: nodejs/node-addon-api#1099
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Nov 6, 2021
@idan-at idan-at marked this pull request as ready for review November 6, 2021 08:41
@targos targos added the node-api Issues and PRs related to the Node-API. label Nov 6, 2021
@targos
Copy link
Member

targos commented Nov 6, 2021

@nodejs/node-api

NickNaso
NickNaso reviewed Nov 6, 2021
View reviewed changes
Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

The new apis should be considered experimental.

src/js_native_api.h Outdated
@@ -111,6 +111,10 @@ NAPI_EXTERN napi_status napi_create_range_error(napi_env env,
napi_value code,
napi_value msg,
napi_value* result);
NAPI_EXTERN napi_status napi_create_syntax_error(napi_env env,
Copy link
Member

Choose a reason for hiding this comment

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

This is a new api and it should be enabled / disabled throw NAPI_EXPERIMENTAL:

#ifdef NAPI_EXPERIMENTAL
NAPI_EXTERN 
napi_status napi_create_syntax_error(napi_env env,
                                     napi_value code,
                                     napi_value msg,
                                     napi_value* result);
#endif  // NAPI_EXPERIMENTAL

src/js_native_api.h Outdated
@@ -370,6 +374,9 @@ NAPI_EXTERN napi_status napi_throw_type_error(napi_env env,
NAPI_EXTERN napi_status napi_throw_range_error(napi_env env,
const char* code,
const char* msg);
NAPI_EXTERN napi_status napi_throw_syntax_error(napi_env env,
Copy link
Member

Choose a reason for hiding this comment

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

This is a new api and it should be enabled / disabled throw NAPI_EXPERIMENTAL:

#ifdef NAPI_EXPERIMENTAL
NAPI_EXTERN 
napi_status napi_throw_syntax_error(napi_env env,
                                    const char* code,
                                    const char* msg,);
#endif  // NAPI_EXPERIMENTAL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, it's here: 6fafc06

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NickNaso I see that adding this ifdef failed the tests on Mac (due to implicit declaration of function 'napi_throw_syntax_error' is invalid in C99 [-Werror,-Wimplicit-function-declaration]), tho not on the other platforms.

I assume it's either that the compiler is stricter on Mac (or defined to be stricter), or that the tests in this setup are not running with NAPI_EXPERIMENTAL.

What do you recommend to do?

Copy link
Member

Choose a reason for hiding this comment

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

You should add this declaration:

#define NAPI_EXPERIMENTAL

to the head of file /test/js-native-api/test_error/test_error.c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NickNaso Thanks. It helped.

The failing action (test-asan / test-asan (pull_request)) seems flaky, I see:

[UNEXPECTED_FAILURE][FAIL] Entry 0 startTime is approximately correct (up to 20ms difference allowed)
assert_approx_equals: expected 4102.578573999926 +/- 20 but got 4077.9045040002093
    at Test.<anonymous> (/home/runner/work/node/node/test/fixtures/wpt/user-timing/mark.any.js:61:8)
    at Test.step (/home/runner/work/node/node/test/fixtures/wpt/resources/testharness.js:2087:25)
    at test (/home/runner/work/node/node/test/fixtures/wpt/resources/testharness.js:557:30)
    at test_mark (/home/runner/work/node/node/test/fixtures/wpt/user-timing/mark.any.js:59:4)
    at /home/runner/work/node/node/test/fixtures/wpt/user-timing/mark.any.js:117:3
Command: /home/runner/work/node/node/out/Release/node  /home/runner/work/node/node/test/wpt/test-user-timing.js mark.any.js

I don't have the permissions to re-run. can you please?

Copy link
Member

Choose a reason for hiding this comment

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

@NickNaso yes, that is what we agreed to do for new functions use node_api instead of napi

It is what we said we'd do in https://nodejs.medium.com/renaming-n-api-to-node-api-27aa8ca30ed8

Please @idan-at could you please rename the functions you added like reported below?

  • node_api_throw_syntax_error
  • node_api_create_syntax_error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course - did it here a9cc563

@idan-at idan-at mentioned this pull request Nov 6, 2021
Closed
@NickNaso
Copy link
Member

NickNaso commented Nov 6, 2021

@gabrielschulhof @mhdawson should the new functions to be called node_api_throw_syntax_error and node_api_create_syntax_error?

@mhdawson
Copy link
Member

mhdawson commented Nov 8, 2021 •
edited
Loading

@NickNaso yes, that is what we agreed to do for new functions use node_api instead of napi

It is what we said we'd do in https://nodejs.medium.com/renaming-n-api-to-node-api-27aa8ca30ed8

NickNaso
NickNaso approved these changes Nov 11, 2021
View reviewed changes
Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

Well done @idan-at

mhdawson
mhdawson reviewed Nov 15, 2021
View reviewed changes
doc/api/n-api.md Outdated Show resolved Hide resolved
mhdawson
mhdawson reviewed Nov 15, 2021
View reviewed changes
doc/api/n-api.md Outdated Show resolved Hide resolved
mhdawson
mhdawson reviewed Nov 15, 2021
View reviewed changes
doc/api/n-api.md Outdated Show resolved Hide resolved
mhdawson
mhdawson reviewed Nov 15, 2021
View reviewed changes
src/js_native_api.h Outdated Show resolved Hide resolved
mhdawson
mhdawson reviewed Nov 15, 2021
View reviewed changes
src/js_native_api.h Outdated Show resolved Hide resolved
mhdawson
mhdawson reviewed Nov 15, 2021
View reviewed changes
src/js_native_api.h Outdated Show resolved Hide resolved
mhdawson
mhdawson reviewed Nov 15, 2021
View reviewed changes
src/js_native_api.h Outdated Show resolved Hide resolved
mhdawson
mhdawson reviewed Nov 15, 2021
View reviewed changes
src/js_native_api_v8.cc Outdated Show resolved Hide resolved
mhdawson
mhdawson reviewed Nov 15, 2021
View reviewed changes
src/js_native_api_v8.cc Outdated Show resolved Hide resolved
mhdawson
mhdawson approved these changes Nov 15, 2021
View reviewed changes
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, thanks @idan-at

@mhdawson
Copy link
Member

mhdawson commented Nov 15, 2021 •
edited
Loading

@Trott I was asking about that lint-md action failure that just says doc/api/n-api.md is not formatted. Please run 'make format-md'. and @richardlau mentioned he thought you added it.

Is there any way to see what needs to change, fix from the UI? I suspect we may run into failures with that job where people have made updates through the UI and then switching to the command line may not be easy for many people.

@mhdawson
Copy link
Member

@Trott since I think I messed up the first at mention.

@mhdawson
squash: run make format -md
8c2a1b3 
Signed-off-by: Michael Dawson <mdawson@devrus.com>
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Nov 16, 2021
Open
40 tasks
@Trott
Copy link
Member

Trott commented Nov 16, 2021

Is there any way to see what needs to change, fix from the UI?

To see the changes, we'd need to output the diffs in

node/tools/lint-md/lint-md.src.mjs

Lines 43 to 47 in 94fa781

if (isDifferent) {
process.exitCode = 1;
const cmd = process.platform === 'win32' ? 'vcbuild' : 'make';
console.error(`${path} is not formatted. Please run '${cmd} format-md'.`);
}

Once the diffs are shown, people can manually fix them in the UI, I suppose.

@nodejs-github-bot
Copy link
Collaborator

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

mhdawson pushed a commit that referenced this pull request Nov 16, 2021
@mhdawson
src,doc: add SyntaxError napi support
4265f27 
Add `napi_create_syntax_error` and `napi_throw_syntax_error`.

Fixes: nodejs/node-addon-api#1099

PR-URL: #40736
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@mhdawson
Copy link
Member

"Landed in 4265f27"

@mhdawson mhdawson closed this Nov 16, 2021
This was referenced Nov 17, 2021
Open
Open
Open
Open
targos pushed a commit that referenced this pull request Nov 21, 2021
@targos
src,doc: add SyntaxError napi support
88b57bc 
Add `napi_create_syntax_error` and `napi_throw_syntax_error`.

Fixes: nodejs/node-addon-api#1099

PR-URL: #40736
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@targos targos mentioned this pull request Nov 26, 2021
Merged
@RaisinTen RaisinTen mentioned this pull request Jan 2, 2022
Closed
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
@danielleadams
src,doc: add SyntaxError napi support
6ef5cee 
Add `napi_create_syntax_error` and `napi_throw_syntax_error`.

Fixes: nodejs/node-addon-api#1099

PR-URL: #40736
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@danielleadams
src,doc: add SyntaxError napi support
df574f3 
Add `napi_create_syntax_error` and `napi_throw_syntax_error`.

Fixes: nodejs/node-addon-api#1099

PR-URL: #40736
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@jasnell jasnell jasnell approved these changes

@NickNaso NickNaso NickNaso approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adding napi_create_syntax_error
7 participants
@idan-at @targos @NickNaso @mhdawson @nodejs-github-bot @Trott @jasnell