Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

perf: remove unnecessary regex s modifier #15766

Conversation

MajedMuhammad
Copy link
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.

Copy link

stackblitz bot commented Jan 31, 2024

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
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
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
Contributor Author

@bluwy, done. You may want to merge.

Copy link
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-dev patak-dev merged commit 8dc1b73 into vitejs:main Feb 21, 2024
11 checks passed
# for free to join this conversation on GitHub. Already have an account? # 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