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: add changes that should have been in 18394 #18509

Closed
wants to merge 1 commit into from
Closed

module: add changes that should have been in 18394 #18509

wants to merge 1 commit into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Feb 1, 2018 •
edited by TimothyGu
Loading

these changes make it so that the new behavior of ModuleWrap::Link will be used by vm.Module and enforced by tests for vm.Module, and as we don't specifically test our internal bindings i think this will be good enough.

See: #18394

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

src, vm

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Feb 1, 2018
@devsnek
Copy link
Member Author

devsnek commented Feb 1, 2018

cc @iamstolis

@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

@devsnek is it possible to add a test for this?

@devsnek
Copy link
Member Author

devsnek commented Feb 1, 2018 •
edited
Loading

@BridgeAR i'm not sure exactly how, we don't expose ModuleWrap itself and generally we test implementations not their native backing right? i originally couldn't figure out how to test this because i wasn't sure how to insert a super long job into the node loader, but its usage in vm.Module ensures the behavior works, which is how i caught that bug with settings the elements of the promise array.

@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

Hm ok. If someone has an idea: I guess it would be nice to add a test just to make sure.

@iamstolis
Copy link

Looks good to me, thanks for the fix.

TimothyGu
TimothyGu reviewed Feb 1, 2018
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I should've caught this sooner... LGTM with tests.

@bnoordhuis
Copy link
Member

...and a better commit log. What changes? Why?

@devsnek
Copy link
Member Author

devsnek commented Feb 1, 2018

afaik there's no way to test internalBinding modules, if anyone has an idea on how to do that please let me know

@targos
Copy link
Member

targos commented Feb 1, 2018

afaik there's no way to test internalBinding modules, if anyone has an idea on how to do that please let me know

You can export the binding from an internal/ module

@devsnek
Copy link
Member Author

devsnek commented Feb 1, 2018 •
edited
Loading

@targos that seems pretty messy, maybe we can provide internalBinding to certain tests by folder or something similar? (perhaps tests/internal, as a separate pr, although i don't want to block this pr, i'm very conflicted rn, i'll add it for this pr and then look into removing it later)

@devsnek devsnek force-pushed the fix/module-linking branch 4 times, most recently from d753441 to 968ca4a Compare February 3, 2018 20:00
@devsnek
module: add changes that should have been in 18394
35a175a 
* switch vm.Module internals to use the new link method properly
* fix bug with ModuleWrap::Link
* add tests for ModuleWrap::Link
@devsnek devsnek force-pushed the fix/module-linking branch from 968ca4a to 35a175a Compare February 3, 2018 20:04
@TimothyGu
Copy link
Member

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

@TimothyGu
Copy link
Member

Landed in b0f114d.

@TimothyGu TimothyGu closed this Feb 4, 2018
@TimothyGu TimothyGu mentioned this pull request Feb 4, 2018
Closed
TimothyGu pushed a commit that referenced this pull request Feb 4, 2018
@devsnek @TimothyGu
loader: fix up #18394
b0f114d 
This commit fixes up some issues in #18394.

* Switch vm.Module internals to use the new link method properly
* Fix bug with ModuleWrap::Link
* Add tests for ModuleWrap::Link

PR-URL: #18509
Fixes: #18249
Refs: #18394
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@devsnek devsnek deleted the fix/module-linking branch February 4, 2018 21:51
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Closed
4 tasks
@MylesBorins
Copy link
Contributor

This should come with #17560 when it is backported

@devsnek
Copy link
Member Author

devsnek commented Feb 21, 2018 •
edited
Loading

this should be directly backported with #18394 not #17560 or the loader will stop working. i'm not sure what to do about the changes to the vm module file

@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
loader: fix up nodejs#18394
f934e4b 
This commit fixes up some issues in nodejs#18394.

* Switch vm.Module internals to use the new link method properly
* Fix bug with ModuleWrap::Link
* Add tests for ModuleWrap::Link

PR-URL: nodejs#18509
Fixes: nodejs#18249
Refs: nodejs#18394
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@devsnek @MylesBorins
loader: fix up #18394
0e105ae 
This commit fixes up some issues in #18394.

* Switch vm.Module internals to use the new link method properly
* Fix bug with ModuleWrap::Link
* Add tests for ModuleWrap::Link

PR-URL: #18509
Fixes: #18249
Refs: #18394
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@devsnek @MylesBorins
loader: fix up #18394
d1d7405 
This commit fixes up some issues in #18394.

* Switch vm.Module internals to use the new link method properly
* Fix bug with ModuleWrap::Link
* Add tests for ModuleWrap::Link

PR-URL: #18509
Fixes: #18249
Refs: #18394
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@devsnek @MylesBorins
loader: fix up #18394
cb36b67 
This commit fixes up some issues in #18394.

* Switch vm.Module internals to use the new link method properly
* Fix bug with ModuleWrap::Link
* Add tests for ModuleWrap::Link

PR-URL: #18509
Fixes: #18249
Refs: #18394
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@devsnek @MayaLekova
loader: fix up nodejs#18394
e618459 
This commit fixes up some issues in nodejs#18394.

* Switch vm.Module internals to use the new link method properly
* Fix bug with ModuleWrap::Link
* Add tests for ModuleWrap::Link

PR-URL: nodejs#18509
Fixes: nodejs#18249
Refs: nodejs#18394
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

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

@TimothyGu TimothyGu TimothyGu approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. 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.
8 participants
@devsnek @BridgeAR @iamstolis @bnoordhuis @targos @TimothyGu @MylesBorins @nodejs-github-bot