Skip to content

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

Closed
lordazzi wants to merge 1 commit into
angular:masterfrom
lordazzi:attribute-regex-fix
Closed

lordazzi wants to merge 1 commit into
angular:masterfrom
lordazzi:attribute-regex-fix

Conversation

@lordazzi

Copy link
Copy Markdown

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

'([-\\w]+)|' + // "tag"
'(?:\\.([-\\w]+))|' + // ".class"
'(?:\\[([.-\\w*]+)(?:=([^\\]]*))?\\])|' + // "[name]", "[name=value]"
'(?:\\[([.\\-\\w*]+)(?:=([^\\]]*))?\\])|' + // "[name]", "[name=value]"

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

vicb commented Jan 12, 2017

Copy link
Copy Markdown
Contributor

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
Copy Markdown

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