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: fix cyclical dynamic import #18965

Closed
wants to merge 1 commit into from
Closed

module: fix cyclical dynamic import #18965

wants to merge 1 commit into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Feb 23, 2018 •
edited
Loading

ensures that instantiation result is only used during initial loading. current loading mechanism would use the empty promise sentinel if the import() fired in a cyclic manner.

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)

module

@bmeck bmeck requested review from jkrems and devsnek February 23, 2018 21:14
@bmeck bmeck force-pushed the fix-cyclic-dynamic-import branch from b6a542f to a3f7f9a Compare February 23, 2018 21:26
@mscdex
Copy link
Contributor

mscdex commented Feb 23, 2018

Typo in commit message

@bmeck bmeck force-pushed the fix-cyclic-dynamic-import branch from 483e758 to 881ae75 Compare February 23, 2018 21:41
@bmeck
module: fix cyclical dynamic import
881ae75 
ensures that instantiation result is only used during initial loading
@mscdex mscdex changed the title module: fix cylcical dynamic import module: fix cyclical dynamic import Feb 23, 2018
Fishrock123
Fishrock123 approved these changes Feb 24, 2018
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

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

@TimothyGu TimothyGu added the esm Issues and PRs related to the ECMAScript Modules implementation. label Feb 27, 2018
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 1, 2018
@bmeck
Copy link
Member Author

bmeck commented Mar 5, 2018

Landed in 023f49c

bmeck added a commit that referenced this pull request Mar 5, 2018
@bmeck
module: fix cyclical dynamic import
023f49c 
ensures that instantiation result is only used during initial loading

PR-URL: #18965
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@joyeecheung joyeecheung closed this Mar 5, 2018
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
@bmeck @MylesBorins
module: fix cyclical dynamic import
3c8f4a2 
ensures that instantiation result is only used during initial loading

PR-URL: #18965
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 6, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
@bmeck @MylesBorins
module: fix cyclical dynamic import
5583547 
ensures that instantiation result is only used during initial loading

PR-URL: #18965
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
@bmeck @MylesBorins
module: fix cyclical dynamic import
bc1038a 
ensures that instantiation result is only used during initial loading

PR-URL: #18965
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 6, 2018
@bmeck @MylesBorins
module: fix cyclical dynamic import
a3b9542 
ensures that instantiation result is only used during initial loading

PR-URL: #18965
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 7, 2018
@bmeck @MylesBorins
module: fix cyclical dynamic import
95f6467 
ensures that instantiation result is only used during initial loading

PR-URL: #18965
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@bmeck @MayaLekova
module: fix cyclical dynamic import
78b6eb9 
ensures that instantiation result is only used during initial loading

PR-URL: nodejs#18965
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@jasnell
Copy link
Member

jasnell commented Aug 17, 2018

Should this be backported to 8.x?

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

@BridgeAR BridgeAR BridgeAR approved these changes

@devsnek devsnek devsnek approved these changes

@cjihrig cjihrig cjihrig approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@guybedford guybedford guybedford approved these changes

@jkrems jkrems jkrems approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.