Handle circular slot dependencies in Modules (fix #4482) #4550
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jackhorton
reviewed
Jan 13, 2018
|
|
||
| uint SourceTextModuleRecord::GetLocalExportSlotIndexByExportName(PropertyId exportNameId) | ||
| { | ||
| if(!this->WasDeclarationInitialized())//allow for circular imports that reference each other's functions |
Contributor
There was a problem hiding this comment.
Nit: coding style isnt a big deal for us, but I think we usually avoid using this-> unless it is to avoid ambiguity.
Collaborator
Author
There was a problem hiding this comment.
Happy to remove though this-> is used 109 times in SourceTextModuleRecord so I was kinda copying what I'd seen there.
Contributor
|
/cc @boingoing , who is the person I know with the most context for ES Modules. |
Collaborator
Author
|
#4551 does this in a neater way so closing this PR |
chakrabot
pushed a commit
that referenced
this pull request
Jan 16, 2018
Merge pull request #4551 from obastemur:module_circular Alternative to #4550 , fixes #4482 Saw the test case brought by @rhuanjl to #4482 and the proposed fix at #4550 . After looking at the code, looks like couple of tiny changes are needed to module support. I don't have much context on modules though. This PR is just a weekend after breakfast hacking. /cc @akroshg @boingoing @rhuanjl #### how it works Separate `ModuleDeclarationInstantiation` into `ModuleDeclarationInstantiation` and `GenerateRootFunction`. This way, in case `childrenModuleSet` has circular dependents, we may instantiate all the modules prior to triggering the rest
chakrabot
pushed a commit
that referenced
this pull request
Jan 16, 2018
Merge pull request #4551 from obastemur:module_circular Alternative to #4550 , fixes #4482 Saw the test case brought by @rhuanjl to #4482 and the proposed fix at #4550 . After looking at the code, looks like couple of tiny changes are needed to module support. I don't have much context on modules though. This PR is just a weekend after breakfast hacking. /cc @akroshg @boingoing @rhuanjl #### how it works Separate `ModuleDeclarationInstantiation` into `ModuleDeclarationInstantiation` and `GenerateRootFunction`. This way, in case `childrenModuleSet` has circular dependents, we may instantiate all the modules prior to triggering the rest
chakrabot
pushed a commit
that referenced
this pull request
Jan 17, 2018
…ircular import Merge pull request #4551 from obastemur:module_circular Alternative to #4550 , fixes #4482 Saw the test case brought by @rhuanjl to #4482 and the proposed fix at #4550 . After looking at the code, looks like couple of tiny changes are needed to module support. I don't have much context on modules though. This PR is just a weekend after breakfast hacking. /cc @akroshg @boingoing @rhuanjl #### how it works Separate `ModuleDeclarationInstantiation` into `ModuleDeclarationInstantiation` and `GenerateRootFunction`. This way, in case `childrenModuleSet` has circular dependents, we may instantiate all the modules prior to triggering the rest
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #4482 and adds the POC of that bug as a test case.
Notes: