Skip to content

perf: remove unnecessary regex s modifier#15766

Merged
patak-cat merged 1 commit intovitejs:mainfrom
MajedMuhammad:bug-fix-regex-'s'-modifier-removal-for-older-browsers-support
Feb 21, 2024
Merged

perf: remove unnecessary regex s modifier#15766
patak-cat merged 1 commit intovitejs:mainfrom
MajedMuhammad:bug-fix-regex-'s'-modifier-removal-for-older-browsers-support

Conversation

@MajedMuhammad
Copy link
Copy Markdown
Contributor

@MajedMuhammad MajedMuhammad commented Jan 31, 2024

Description

Minor bug fix:

Fixes bug caused by this commit: 2687dbb
It is just a regex equivalent replacement:
. with s modifier is 100% equal to [\s\S] without s modifier.

s modifier is causing older browsers to fail with a blank page with console error such as:
SyntaxError: invalid regular expression flag s

Future Consideration

Please avoid accepting PRs that include commits including regex s modifier/flag.


What is the purpose of this pull request?

  • Bug fix X
  • New Feature
  • Documentation update
  • Other

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

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

@bolt-new-by-stackblitz
Copy link
Copy Markdown

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@MajedMuhammad MajedMuhammad changed the title bug fix: regex 's' modifier removal for older browsers support fix: regex 's' modifier removal for older browsers support Jan 31, 2024
@MajedMuhammad
Copy link
Copy Markdown
Contributor Author

@patak-dev, sorry for mentioning you for a very minor bug.
However, you may want to accept this PR, it is nothing, but it just keeps support for legacy browsers.
No impact at all, it is equivalent.

@bluwy
Copy link
Copy Markdown
Member

bluwy commented Feb 8, 2024

The browsers supported by Vite should also support the s modifier, so I don't think we should avoid using this because of that.

But now that you brought this up, it doesn't look like we need to use the s modifier in the first place. The strings we're dealing with should always only be a single line. I think we can simply remove the s modifier as a perf improvement, and we can do the same for the others: https://github.com/search?q=repo%3Avitejs%2Fvite%20%2F%5B%3F%23%5D.*%24%2Fs&type=code

@MajedMuhammad MajedMuhammad deleted the bug-fix-regex-'s'-modifier-removal-for-older-browsers-support branch February 13, 2024 18:45
@MajedMuhammad MajedMuhammad restored the bug-fix-regex-'s'-modifier-removal-for-older-browsers-support branch February 13, 2024 18:46
@MajedMuhammad MajedMuhammad reopened this Feb 13, 2024
@MajedMuhammad MajedMuhammad force-pushed the bug-fix-regex-'s'-modifier-removal-for-older-browsers-support branch from a4413af to ba8192e Compare February 14, 2024 18:44
@MajedMuhammad
Copy link
Copy Markdown
Contributor Author

@bluwy, done. You may want to merge.

Copy link
Copy Markdown
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Thanks!

@bluwy bluwy changed the title fix: regex 's' modifier removal for older browsers support perf: remove unnecessary regex s modifier Feb 21, 2024
@bluwy bluwy added the p2-nice-to-have Not breaking anything but nice to have (priority) label Feb 21, 2024
@patak-cat patak-cat merged commit 8dc1b73 into vitejs:main Feb 21, 2024
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.

3 participants