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

Fix implementation of napi_create_boolean #31

Conversation

gabrielschulhof
Copy link
Collaborator

This is a leftover copy/paste error that causes booleans to be rendered as numbers when they return to JS.

@digitalinfinity
Copy link
Contributor

LGTM - thanks for fixing! cc @boingoing

mhdawson

This comment was marked as off-topic.

@mhdawson
Copy link
Member

What about the similar fix for the other branches ? We at least need it in the 6.2 branch as I'm thinking we'll use that as the abi-stable-node "master" when we want to rebase onto the latest node.

@gabrielschulhof
Copy link
Collaborator Author

@mhdawson this fix is chakracore-specfic, and AFAICT api-prototype-chakracore-7.x is the only branch that contains chakracore, and therefore src/node_jsrtapi.cc. Thus, there should be no other branches to which this fix needs to be applied.

@mhdawson
Copy link
Member

@gabrielschulhof sorry should have looked more closely at what file was being changed.

@gabrielschulhof
Copy link
Collaborator Author

@mhdawson NP :) I too have often confused node_jsvmapi.cc with node_jsrtapi.cc.

@ianwjhalliday
Copy link

ianwjhalliday commented Jan 5, 2017

I was not happy with how those file names turned out. Would be great for someone to reorganize or rename them to clearly show they are v8 and jsrt implementations of napi. node_napi_v8.cc and node_napi_jsrt.cc?

(drive by holla')

@gabrielschulhof
Copy link
Collaborator Author

Do I need to do anything with this PR before it can be merged?

gabrielschulhof pushed a commit that referenced this pull request Jan 16, 2017
Need to call JsBoolToBoolean(), not JsDoubleToNumber() when creating a
JS boolean from a bool.

Closes: #31
gabrielschulhof pushed a commit that referenced this pull request Jan 16, 2017
Need to call JsBoolToBoolean(), not JsDoubleToNumber() when creating a
JS boolean from a bool.

Closes #31
@gabrielschulhof gabrielschulhof force-pushed the api-prototype-chakracore-7.x branch from 824d477 to e2480ba Compare January 16, 2017 08:54
gabrielschulhof pushed a commit that referenced this pull request Jan 16, 2017
Need to call JsBoolToBoolean(), not JsDoubleToNumber() when creating a
JS boolean from a bool.

Closes: #31
@gabrielschulhof
Copy link
Collaborator Author

Closed in e2480ba

jasongin pushed a commit that referenced this pull request Jan 25, 2017
Need to call JsBoolToBoolean(), not JsDoubleToNumber() when creating a
JS boolean from a bool.

Closes: #31
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.

4 participants