-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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(analysis): warnings for dynamic imports that use static template literals #14458
Conversation
Run & review this pull request in StackBlitz Codeflow. |
1148b7d
to
8509b58
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.
I like this change!
} else if (templateLiteralRE.test(rawUrl)) { | ||
// Only static template literal will into this branch. | ||
// It has variables will processed in importMetaGlob.ts | ||
specifier = rawUrl.replace(templateLiteralRE, '$1') |
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 don't think this is correct.
Dynamic import with /* @vite-ignore */
or dynamic imports that cannot be handled with the dynamic-import plugin (e.g. import(`${startsWithVariable}/foo`)
) will be still there.
https://stackblitz.com/edit/vitejs-vite-fnln8a?file=main.js&terminal=dev
So templateLiteralRE.test(rawUrl)
will match `foo${va}`
https://stackblitz.com/edit/node-ptwy2i?file=index.js
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
Oh, I realize that. A dynamic import that doesn't conform to the rules will still get in here. So do I just need to exclude paths containing variables here?
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.
So do I just need to exclude paths containing variables here?
I think so 👍
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.
import(`foo\${bar}.js`)
will still cause a warning but I guess that's fine.
…literals (#14458) Co-authored-by: 翠 / green <[email protected]>
…literals (#14458) Co-authored-by: 翠 / green <[email protected]>
Description
When I finished this PR, I noticed that there was another one #14103 being processed. But I think we shouldn't hide warnings.
Fixes: #14101
Closes: #14103
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).