Skip to content
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: regexp perf issues, refactor regexp stylistic issues #10905

Merged
merged 10 commits into from Nov 14, 2022

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Nov 13, 2022

Description

Add eslint-plugin-regexp to detect some perf issues.

I guess this will fix #10900 along the way. Actually it wasn't catched by eslint-plugin-regexp. But I included a fix for it in this PR.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@sapphi-red sapphi-red marked this pull request as ready for review November 14, 2022 11:17
@sapphi-red sapphi-red changed the title fix: regexp perf issues, refactor regexp stylistic issues perf: regexp perf issues, refactor regexp stylistic issues Nov 14, 2022
@@ -40,7 +40,7 @@ const htmlTypesRE = /\.(html|vue|svelte|astro|imba)$/
// since even missed imports can be caught at runtime, and false positives will
// simply be ignored.
export const importsRE =
/(?<!\/\/.*)(?<=^|;|\*\/)\s*import(?!\s+type)(?:[\w*{}\n\r\t, ]+from\s*)?\s*("[^"]+"|'[^']+')\s*(?=$|;|\/\/|\/\*)/gm
/(?<!\/\/.*)(?<=^|;|\*\/)\s*import(?!\s+type)(?:[\w*{}\n\r\t, ]+from)?\s*("[^"]+"|'[^']+')\s*(?=$|;|\/\/|\/\*)/gm
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This \s* can be matched by the next \s*.

@@ -149,13 +149,13 @@ function globEntries(pattern: string | string[], config: ResolvedConfig) {
}

const scriptModuleRE =
/(<script\b[^>]*type\s*=\s*(?:"module"|'module')[^>]*>)(.*?)<\/script>/gims
export const scriptRE = /(<script\b(?:\s[^>]*>|>))(.*?)<\/script>/gims
/(<script\b[^>]+type\s*=\s*(?:"module"|'module')[^>]*>)(.*?)<\/script>/gis
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because \b exists [^>] will always match more than one char.

/(<script\b[^>]*type\s*=\s*(?:"module"|'module')[^>]*>)(.*?)<\/script>/gims
export const scriptRE = /(<script\b(?:\s[^>]*>|>))(.*?)<\/script>/gims
/(<script\b[^>]+type\s*=\s*(?:"module"|'module')[^>]*>)(.*?)<\/script>/gis
export const scriptRE = /(<script(?:\s[^>]*>|>))(.*?)<\/script>/gis
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because \s and > both matches \b, this \b isn't needed.

@@ -38,7 +38,7 @@ export function assetImportMetaUrlPlugin(config: ResolvedConfig): Plugin {
) {
let s: MagicString | undefined
const assetImportMetaUrlRE =
/\bnew\s+URL\s*\(\s*('[^']+'|"[^"]+"|`[^`]+`)\s*,\s*import\.meta\.url\s*,?\s*\)/g
/\bnew\s+URL\s*\(\s*('[^']+'|"[^"]+"|`[^`]+`)\s*,\s*import\.meta\.url\s*(?:,\s*)?\)/g
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\s*,?\s* is same with \s*(?:,\s*)?.

Comment on lines -1178 to +1181
/(?<=^|[^\w\-\u0080-\uffff])url\(\s*('[^']+'|"[^"]+"|[^'")]+)\s*\)/
/(?<=^|[^\w\-\u0080-\uffff])url\((\s*('[^']+'|"[^"]+")\s*|[^'")]+)\)/
export const cssDataUriRE =
/(?<=^|[^\w\-\u0080-\uffff])data-uri\(\s*('[^']+'|"[^"]+"|[^'")]+)\s*\)/
/(?<=^|[^\w\-\u0080-\uffff])data-uri\((\s*('[^']+'|"[^"]+")\s*|[^'")]+)\)/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\s*('[^']+'|"[^"]+"|[^'")]+)\s* is same with (\s*('[^']+'|"[^"]+")\s*|[^'")]+). (String::trim() needs to be run later.)

Comment on lines -1394 to +1397
/@import\s*(?:url\([^\)]*\)|"([^"]|(?<=\\)")*"|'([^']|(?<=\\)')*'|[^;]*).*?;/gm
/@import(?:\s*(?:url\([^)]*\)|"(?:[^"]|(?<=\\)")*"|'(?:[^']|(?<=\\)')*').*?|[^;]*);/g
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[^;]* matches \s*. So splitted [^;]*.

@@ -20,7 +20,7 @@ export function ssrRewriteStacktrace(
.split('\n')
.map((line) => {
return line.replace(
/^ {4}at (?:(.+?)\s+\()?(?:(.+?):(\d+)(?::(\d+))?)\)?/,
/^ {4}at (?:(\S.*?)\s\()?(.+?):(\d+)(?::(\d+))?\)?/,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(.+?)\s+ is same with (\S.*?)\s. (String::trim() needs to be run later.)

@patak-dev patak-dev enabled auto-merge (squash) November 14, 2022 12:16
@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Nov 14, 2022
@patak-dev patak-dev merged commit fc007df into vitejs:main Nov 14, 2022
@sapphi-red sapphi-red deleted the fix/eslint-plugin-regexp branch November 14, 2022 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regex for injecting UMD helpers makes vite build terribly slow
2 participants