-
Notifications
You must be signed in to change notification settings - Fork 483
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: align parser.walk_expression
to webpack, which put into context_dependency_helper
#6963
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for rspack canceled.Built without sensitive environment variables
|
daba9d5
to
3cdae65
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.
Thank you!
Do you need a double check? @ahabhgk |
} | ||
|
||
impl<'a, F: FnMut(&Expr)> Visit for BasicEvaluatedExpressionVisitor<'a, F> { | ||
fn visit_expr(&mut self, n: &Expr) { |
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.
Walk is different with swc visitor, parser plugins' goal is replace all the swc visitors, so this needs to rewrite to walk_expression
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 use this visitor to find the Expr
ast node corresponding to the BasicEvaluatedExpression and then call parser.walk_expression
. how would this visitor be rewritten to walk_expression
?
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 think there could have some walk around to implement this and avoid the life time issue, I will take a look next week
Summary
fix #6874
The cause of the problem is the direct walking of import call expression, which led to a conflict between the replacement generated by this walk and the replacement handled by ConstPlugin.
Webpack handles the import call expression in three cases:
TemplateLiteral: walk the parts with odd indices, see https://github.com/webpack/webpack/blob/8241da7f1e75c5581ba535d127fa66aeb9eb2ac8/lib/dependencies/ContextDependencyHelpers.js#L151
Wrapped expr: process the inner expressions, see https://github.com/webpack/webpack/blob/8241da7f1e75c5581ba535d127fa66aeb9eb2ac8/lib/dependencies/ContextDependencyHelpers.js#L218
Other cases: directly walk the import call expression, see https://github.com/webpack/webpack/blob/8241da7f1e75c5581ba535d127fa66aeb9eb2ac8/lib/dependencies/ContextDependencyHelpers.js#L241
My solution is to use a visitor to traverse the expression and find the corresponding
Expr
forBasicEvaluatedExpressionVisitor
based on span. A better solution might be to add a reference to the correspondingExpr
forBasicEvaluatedExpressionVisitor
like webpack does. I tried this but couldn't resolve the lifetime issues.Checklist