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

vm: flip Module#link's signature #18471

Closed
wants to merge 1 commit into from
Closed

vm: flip Module#link's signature #18471

wants to merge 1 commit into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Jan 31, 2018

just a nit i should have fixed in the original pr but unfortunately i didn't catch this. basically while linking you 100% want the specifier but you might not need the referring module depending on how your linking is set up, so these args are better flipped

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

vm

@nodejs-github-bot nodejs-github-bot added the vm Issues and PRs related to the vm subsystem. label Jan 31, 2018
@BridgeAR BridgeAR added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 1, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

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

@TimothyGu
Copy link
Member

Landed in 6d84ece.

@TimothyGu TimothyGu closed this Feb 3, 2018
TimothyGu pushed a commit that referenced this pull request Feb 3, 2018
@devsnek @TimothyGu
vm: flip Module#link's signature
6d84ece 
The specifier parameter is deemed to be more essential than
referencingModule. Flipping the parameter order allows developers to
write simple linker functions that only take in a specifier.

PR-URL: #18471
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@devsnek devsnek deleted the nit/vm-module-link-signature branch February 3, 2018 19:30
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2018
@MylesBorins
Copy link
Contributor

I think, but not 100%, that this depends on a semver major change. If so please change to labels to do-not-land... if not perhaps a backport?

@devsnek
Copy link
Member Author

devsnek commented Feb 21, 2018 •
edited
Loading

this has no semver because vm.module is still experimental, i'll backport it

@devsnek devsnek mentioned this pull request Feb 21, 2018
Closed
2 tasks
devsnek added a commit to devsnek/node that referenced this pull request Feb 21, 2018
@devsnek
vm: flip Module#link's signature
c401321 
The specifier parameter is deemed to be more essential than
referencingModule. Flipping the parameter order allows developers to
write simple linker functions that only take in a specifier.

PR-URL: nodejs#18471
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@devsnek @MylesBorins
vm: flip Module#link's signature
837c91d 
The specifier parameter is deemed to be more essential than
referencingModule. Flipping the parameter order allows developers to
write simple linker functions that only take in a specifier.

PR-URL: #18471
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@devsnek @MylesBorins
vm: flip Module#link's signature
1a0168e 
The specifier parameter is deemed to be more essential than
referencingModule. Flipping the parameter order allows developers to
write simple linker functions that only take in a specifier.

PR-URL: #18471
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@devsnek @MylesBorins
vm: flip Module#link's signature
02afdbc 
The specifier parameter is deemed to be more essential than
referencingModule. Flipping the parameter order allows developers to
write simple linker functions that only take in a specifier.

PR-URL: #18471
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@devsnek @MayaLekova
vm: flip Module#link's signature
06a4261 
The specifier parameter is deemed to be more essential than
referencingModule. Flipping the parameter order allows developers to
write simple linker functions that only take in a specifier.

PR-URL: nodejs#18471
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Contributor

Should we backport to v8.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
fast-track PRs that do not need to wait for 48 hours to land. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@devsnek @BridgeAR @TimothyGu @MylesBorins @jasnell @targos @cjihrig @addaleax @nodejs-github-bot