Skip to content

src: add SyntaxError #1326

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
Merged
merged 1 commit into from
Jun 16, 2023
Merged

src: add SyntaxError #1326

merged 1 commit into from
Jun 16, 2023

Conversation

@KevinEady
Copy link
Contributor

@KevinEady KevinEady commented Jun 10, 2023

Adds wrapper for Napi::SyntaxError from release of Node-API v9.

@KevinEady
Copy link
Contributor Author

KevinEady commented Jun 10, 2023 •
edited
Loading

Node Version Link Status
20 https://ci.nodejs.org/job/node-test-node-addon-api-new/7558/ ✅
21 https://ci.nodejs.org/job/node-test-node-addon-api-new/7559/ ✅*

*: osx1015 failing due to #1328

mhdawson
mhdawson reviewed Jun 12, 2023
View reviewed changes
doc/syntax_error.md
@@ -0,0 +1,66 @@
# SyntaxError

Copy link
Member

@mhdawson mhdawson Jun 12, 2023

Choose a reason for hiding this comment

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

Should it say here that it's only supported in node-api version 9 and higher?

Copy link
Contributor Author

@KevinEady KevinEady Jun 12, 2023

Choose a reason for hiding this comment

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

I don't think so... We don't have any other node-api version information in our docs, eg. Napi::Object::Freeze() does not mention being Node-API v8 only.

Copy link
Member

@mhdawson mhdawson Jun 12, 2023

Choose a reason for hiding this comment

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

That might be a gap though ? I go to try to use an API and it's not there? Either way agree that we don't need to be block this PR on it, but might be worth thinking about wether adding that info makes sense.

mhdawson
mhdawson approved these changes Jun 12, 2023
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 with a suggestion

@KevinEady KevinEady mentioned this pull request Jun 12, 2023
Merged
legendecas
legendecas approved these changes Jun 13, 2023
View reviewed changes
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 % one nit.

test/error.js

module.exports = require('./common').runTestWithBindingPath(test);

const napiVersion = Number(process.env.NAPI_VERSION ?? process.versions.napi);
Copy link
Member

@legendecas legendecas Jun 13, 2023

Choose a reason for hiding this comment

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

It might be better to move the napiVersion definition in test/index.js to test/common/index.js so that we don't have to repeat it.

@KevinEady
Copy link
Contributor Author

KevinEady commented Jun 13, 2023

Added do not land label until release of v7.0.0 is complete.

@KevinEady KevinEady merged commit d9828c6 into nodejs:main Jun 16, 2023
@KevinEady KevinEady deleted the add-syntaxerror branch June 16, 2023 21:35
@snyk-io snyk-io bot mentioned this pull request Nov 10, 2024
Open
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

@legendecas legendecas legendecas approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@KevinEady @legendecas @mhdawson