Skip to content
This repository has been archived by the owner on Jun 18, 2021. It is now read-only.

Use nan helpers to set exports #125

Merged
merged 1 commit into from Mar 31, 2019
Merged

Conversation

richardlau
Copy link
Member

@richardlau richardlau commented Mar 29, 2019

Fixes compilation errors when built on Node.js master with V8 7.4.

Fixes: #116

@richardlau richardlau mentioned this pull request Mar 29, 2019
@sam-github
Copy link
Contributor

Why'd you change the case of the method names?

@richardlau
Copy link
Member Author

richardlau commented Mar 29, 2019

Why'd you change the case of the method names?

Because

  exports->Set(Nan::New("triggerReport").ToLocalChecked(),
               Nan::New<v8::FunctionTemplate>(TriggerReport)->GetFunction());

exports to JavaScript land a function called triggerReport (starting with a lower case letter) mapping to TriggerReport (the C++ method).

The replacement NAN_EXPORT(target, triggerReport); exports to JavaScript land a function with the same name as the C++ method so they must match case. Changing the case of the function exposed to JavaScript would be a semver major change (e.g. we document (https://github.com/nodejs/node-report#configuration) requiring 'node-report/api') so I changed the C++ side instead.

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

mit: I suggest the commit message say why use of NAN_EXPORT() requires changing the function name.

@richardlau
Copy link
Member Author

richardlau commented Mar 29, 2019

mit: I suggest the commit message say why use of NAN_EXPORT() requires changing the function name.

@sam-github How does the following sound?

NAN_EXPORT() requires the function name exported to JavaScript land and the exported C++ method to match. Change the exported C++ methods to begin with a lower case character to preserve the existing casing of the name exported to JavaScript.

@sam-github
Copy link
Contributor

Very clear, thanks.

Fixes compilation errors when built on Node.js master with V8 7.4.

`NAN_EXPORT()` requires the function name exported to JavaScript land
and the exported C++ method to match. Change the exported C++ methods
to begin with a lower case character to preserve the existing casing of
the name exported to JavaScript.

PR-URL: nodejs#125
Fixes: nodejs#116
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants