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

src: enable wrapping Napi namespace with custom namespace #1135

Merged
merged 1 commit into from Feb 22, 2022

Conversation

addaleax
Copy link
Member

Enable support for a NAPI_CPP_CUSTOM_NAMESPACE that would wrap
the Napi namespace in a custom namespace.

This can be helpful when there are multiple parts of a compiled
shared library or executable that depend on node-addon-api,
but different versions of it. In order to avoid symbol conflicts
in these cases, namespacing can be used to make sure that the
linker would not merge definitions that the compiler potentially
generates for the node-addon-api methods.

Enable support for a `NAPI_CPP_CUSTOM_NAMESPACE` that would wrap
the `Napi` namespace in a custom namespace.

This can be helpful when there are multiple parts of a compiled
shared library or executable that depend on `node-addon-api`,
but different versions of it. In order to avoid symbol conflicts
in these cases, namespacing can be used to make sure that the
linker would not merge definitions that the compiler potentially
generates for the node-addon-api methods.
@addaleax
Copy link
Member Author

Eslint error: 
/home/runner/work/node-addon-api/node-addon-api/test/error_terminating_environment.js
  38:3  error  Parsing error: 'return' outside of function

that seems silly, since I didn’t touch that part, and it’s perfectly valid to do top-level return in Node.js?

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

@mhdawson
Copy link
Member

mhdawson commented Feb 21, 2022

@addaleax

that seems silly, since I didn’t touch that part, and it’s perfectly valid to do top-level return in Node.js?

When we enabled eslint, it was set up to report on changed files. It has been awkward at times but avoided the need to do a massive change at one time.

Can you try adding

// eslint-disable-next-line

before the reported line to see if that quiets the report?

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.

LGTM

@addaleax
Copy link
Member Author

before the reported line to see if that quiets the report?

That doesn’t work because this is a parsing error, not a linting error.

I’ve opened #1141, that should avoid this issue in the future.

@addaleax addaleax merged commit 5b51864 into main Feb 22, 2022
@addaleax addaleax deleted the custom-namespace branch February 22, 2022 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants