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.

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

module: cache and return effective module exports from linked bindings on first call #5337

Closed
wants to merge 1 commit into from
Closed

module: cache and return effective module exports from linked bindings on first call #5337

wants to merge 1 commit into from

Conversation

kaero
Copy link
Contributor

@kaero kaero commented Feb 20, 2016

Related to fix #4771 of #4756

In #4771 the correct exports object goes to the cache, but on the first call of LinkedBinding another (precreated) exports object returned as result. 😐

@mscdex mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. labels Feb 20, 2016
@bnoordhuis
Copy link
Member

LGTM and good catch. The only nit I have is about the commit log: the first line should be <= 50 columns and one or two lines describing the issue and the fix would be appreciated.

CI: https://ci.nodejs.org/job/node-test-pull-request/1709/

module: return correct exports from linked binding
e29fdaa 
Method `LinkedBinding` must return the same "exports" property
of the module as cached, because property can be overridden
by an initialization function.
@kaero
Copy link
Contributor Author

kaero commented Feb 20, 2016

@bnoordhuis 👍 done

@bnoordhuis
Copy link
Member

Thanks, landed in 5c14efb.

@bnoordhuis bnoordhuis closed this Feb 20, 2016
bnoordhuis pushed a commit that referenced this pull request Feb 20, 2016
@bnoordhuis
module: return correct exports from linked binding
5c14efb 
Method `LinkedBinding` must return the same "exports" property
of the module as cached, because property can be overridden
by an initialization function.

PR-URL: #5337
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@thefourtheye
Copy link
Contributor

Shouldn't there be a test?

@bnoordhuis
Copy link
Member

@thefourtheye It's hard to write a test for. process._linkedBinding() is intended for embedders that link in third-party C++ code from lib/_third_party_main.js.

@rvagg
Copy link
Member

rvagg commented Feb 21, 2016

tagged as dont-land-on-v5.x cause it builds on #4771 which is semver-major

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kaero @bnoordhuis @thefourtheye @rvagg @mscdex @targos