-
-
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
perf: remove regex in ImportMetaURL plugins #12502
Conversation
Run & review this pull request in StackBlitz Codeflow. |
@@ -52,7 +56,7 @@ export function assetImportMetaUrlPlugin(config: ResolvedConfig): Plugin { | |||
if (!s) s = new MagicString(code) | |||
|
|||
// potential dynamic template string | |||
if (rawUrl[0] === '`' && /\$\{/.test(rawUrl)) { | |||
if (rawUrl[0] === '`' && placeholderRE.test(rawUrl)) { |
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.
Maybe we can use rawUrl.includes('${')
instead?
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 would also expect this to be faster, but if the regex is extracted it isn't clear: benchmark. I'll replace it anyways but I would love to know why it isn't always faster.
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.
The regex is consistently faster on chrome on M1 🤔
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.
It seems .includes
is consistently faster on my machine even if the regex is extracted. 🤔
I'm not sure about the reason though.
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.
Interesting 👀
Ok, switched to includes
as I also expect that to be faster.
Also did the same for the worker plugin
We can't extract these regexes, as there are |
Ah, that's a good catch. Maybe we can extract them if we replaced |
Description
One of the regex is expensive to generate
What is the purpose of this pull request?