-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
fix(dev): make query selector regexes more inclusive (fix #19213) #19767
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
Conversation
| const htmlProxyRE = /[?&]html-proxy\b/ | ||
| const htmlProxyIndexRE = /&index=(\d+)/ | ||
| const commonjsProxyRE = /\?commonjs-proxy/ | ||
| const commonjsProxyRE = /[?&]commonjs-proxy/ |
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.
this looks to be the only other place where such bug is present
| const htmlProxyRE = /[?&]html-proxy\b/ | ||
| const htmlProxyIndexRE = /&index=(\d+)/ | ||
| const commonjsProxyRE = /\?commonjs-proxy/ | ||
| const commonjsProxyRE = /[?&]commonjs-proxy/ |
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 noticed another possible bug:
Vite built-in plugins use a convention of appending query-like arguments to import specifiers to update the plugin behavior (example: ?url)
Custom plugins may also adapt this convention.
However, if ? character is present in the import specifier, some parts of vite code incorrectly consider it to be a glob pattern:
| if (isDynamicPattern(file)) { |
| if (isDynamicPattern(id)) { |
vite/packages/vite/src/node/optimizer/scan.ts
Line 341 in 2c4a092
| if (resolvedPatterns.every((str) => !isDynamicPattern(str))) { |
I will refactor my plugin to not use the ? convention
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.
However, if
?character is present in the import specifier, some parts of vite code incorrectly consider it to be a glob pattern:
All these codes receives globs (including normal file paths) instead of a list of ids (import specifiers). So it should be fine to use ?s in ids.
sapphi-red
left a comment
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.
Thanks!
|
Thank you for the review |
|
We need 1 more reviews before merging as we normally require 2 or more reviews. |
Description
Fixes #19213
abstract from bug description: