-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix namespace tracing #2408
Fix namespace tracing #2408
Conversation
389f272
to
eb2f9e9
Compare
7694c44
to
285ab76
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice getting to the bottom of this! Just one comment regarding encapsulation.
private referencedEarly: boolean = false; | ||
private references: Identifier[] = []; | ||
|
||
constructor(context: AstContext) { | ||
constructor(context: AstContext, module: Module) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not like adding the module as a parameter here as this breaks the encapsulation created by the AstContext. As the AstContext
already has a traceExport
function, maybe this can be avoided by adding a new method to the NamespaceVariable that does a trace inside this variable's module using its AstContext's traceExport function? This should cover the only usage of .module
in Chunk
while keeping encapsulation of the AST from the module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree about the encapsulation point, as I think it should be possible to trace up the parent hierarchy for any object. Plain data structures will always be a better abstraction than function closures. Yes we're exposing a class, but we're really after the "data representing the module semantically".
The additional point here is that there are two traceExport
functions - one for Module which does direct tracing, and one for Chunk which does chunk to chunk tracing. I did initially spend many hours trying to make these share the same routine but in the end it was much easier this way around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Yes, switching to plain data structures would definitely be a great improvement as it would also allow nice serializations etc. As you have guessed, my issue is with having the "rich" data structure of the module attached to the namespace variable while we go to great lengths to encapsulate the Module methods from the AST in the context.
Then at least we should add a comment to the module
field that it is only meant to be used for tracing exports in the Chunk to avoid people in the future actually accessing properties on it to make other things "work" instead of thinking about the architecture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added a clarifying comment
This PR contains:
Breaking Changes?
Please Describe Your Changes
This resolves #2391 by ensuring that namespace exports are in turn traced in chunking to ensure they are properly resolved.