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

n-api: allow escape of undefined #19434

Closed
wants to merge 1 commit into from
Closed

n-api: allow escape of undefined #19434

wants to merge 1 commit into from

Conversation

mhdawson
Copy link
Member

The node-addon-api module was calling escape on undefined
which would fail. Instead of forcing a check in all consumers
of napi_escape_handle accept undefined and simply return
it as it does not need to be escaped.

Refs: nodejs/node-addon-api#233

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

The node-addon-api module was calling escape on undefined
which would fail. Instead of forcing a check in all consumers
of napi_escape_handle accept undefined and simply return
it as it does not need to be escaped.

Refs: nodejs/node-addon-api#233
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x node-api Issues and PRs related to the Node-API. labels Mar 18, 2018
@addaleax
Copy link
Member

This seems odd … V8 should not make a difference between escaping undefined and other values. In fact, the test code that’s being added here passes on Node 9 for me?

@mhdawson
Copy link
Member Author

@addaleax thanks, I'll take another look. Adding the check in place node-addon-api to avoid the escape resolved the issue in #233 and I figured doing it closer to the source would be better. Maybe it was something in the escape method in node-addon-api as opposed to node_escape_handle.

@mhdawson
Copy link
Member Author

mhdawson commented Apr 2, 2018

So submitting after putting it together on the plane late night seems to have been a bad idea. I see I used a branch in the node repo instead of my own and that I made it undefined instead of null which was the problem.

Since escaping NULL probably is an error, I'll change the code in node-add-api instead. It requires changing in more places but is probably the right thing to do.

@mhdawson mhdawson closed this Apr 2, 2018
@TimothyGu TimothyGu deleted the escape_undefined branch April 15, 2018 04:02
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++. 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