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

fix: escape hyphen in regexp, allow unit tests run in old browsers #13900

Closed
wants to merge 1 commit into from
Closed

fix: escape hyphen in regexp, allow unit tests run in old browsers #13900

wants to merge 1 commit into from

Conversation

lordazzi
Copy link

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
Can't run unit tests over Karma on Firefox 31 (our environment) just because one not escaped character inside a regular expression.

What is the new behavior?
Can run the unit tests normally and get the test report.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

@@ -12,7 +12,7 @@ const _SELECTOR_REGEXP = new RegExp(
'(\\:not\\()|' + //":not("
'([-\\w]+)|' + // "tag"
'(?:\\.([-\\w]+))|' + // ".class"
'(?:\\[([.-\\w*]+)(?:=([^\\]]*))?\\])|' + // "[name]", "[name=value]"
'(?:\\[([.\\-\\w*]+)(?:=([^\\]]*))?\\])|' + // "[name]", "[name=value]"
Copy link
Contributor

Choose a reason for hiding this comment

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

If this needs to be escaped, it is a bug in the browser, please link to so that we do not remove the \\ by accident later on.

Copy link
Author

Choose a reason for hiding this comment

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

You mean to open a issue and link to it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there must be one already, please try to find it and link there.
At the very least add a comment

Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively, could you try [-.\\w*], ie move the - first

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it make sense, I'll do that

@vicb
Copy link
Contributor

vicb commented Jan 12, 2017

Also please run gulp format to format the code.

@vicb vicb added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 12, 2017
vicb added a commit to vicb/angular that referenced this pull request Jan 24, 2017
alxhub pushed a commit that referenced this pull request Jan 24, 2017
alxhub pushed a commit that referenced this pull request Jan 25, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019
idlechara pushed a commit to idlechara/angular that referenced this pull request Apr 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants