★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/18509Nouvelle recherche | Portail wanayoo
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

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

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++ vm labels Feb 1, 2018
@devsnek
Copy link
Member Author

@devsnek devsnek commented Feb 1, 2018

cc @iamstolis

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 1, 2018

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

@devsnek
Copy link
Member Author

@devsnek devsnek commented Feb 1, 2018 •
edited

@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 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

@iamstolis iamstolis commented Feb 1, 2018

Looks good to me, thanks for the fix.

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

@TimothyGu TimothyGu left a comment •
edited

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

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Feb 1, 2018

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

@devsnek
Copy link
Member Author

@devsnek 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 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 devsnek commented Feb 1, 2018 •
edited

@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 Feb 3, 2018
@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
@TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Feb 3, 2018

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

@TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Feb 4, 2018

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 issue 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 <timothygu99@gmail.com>
@devsnek devsnek deleted the fix/module-linking branch Feb 4, 2018
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Closed
4 tasks
@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Feb 21, 2018

This should come with #17560 when it is backported

@devsnek
Copy link
Member Author

@devsnek devsnek commented Feb 21, 2018 •
edited

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 issue 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 <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this issue 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 <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this issue 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 <timothygu99@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this issue 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 <timothygu99@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue 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 <timothygu99@gmail.com>
@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Aug 7, 2018

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

Assignees
No one assigned
Labels
c++ vm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@devsnek @BridgeAR @iamstolis @bnoordhuis @targos @TimothyGu @MylesBorins @nodejs-github-bot