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

Do not escape / #81

Merged
merged 1 commit into from
Jun 16, 2015
Merged

Do not escape / #81

merged 1 commit into from
Jun 16, 2015

Conversation

benjamingr
Copy link
Contributor

Hey, updating the polyfill based on change in https://github.com/benjamingr/RexExp.escape, based on usage data escaping it isn't required (see the data folder for more motivation for this decision).

Hey, updating the polyfill based on change in https://github.com/benjamingr/RexExp.escape, based on usage data escaping it isn't required (see the data folder for more motivation for this decision).
@zloirock
Copy link
Owner

This behavior was based on your polyfill.js, tc39/proposal-regex-escaping#12 (comment) , initially I thought you forgot add it to proposal text. Ok, I'll merge it, but my position - better escape it for code generation.

zloirock added a commit that referenced this pull request Jun 16, 2015
@zloirock zloirock merged commit e062c4d into zloirock:master Jun 16, 2015
@benjamingr
Copy link
Contributor Author

@zloirock yes, this changed in the polyfill.js file too. If you have a compelling case for escaping it or can show me a code base in the wild that does this I'm all ears for this. This of course equally goes for any other character that might need escaping.

I've looked at over 100 thousand code bases in the wild (repos) and found less than 10 usages (the data and scripts used to generate it are open in the repo). People escaping with a .replace or a shim and then using the regexp constructor was a lot more common.

@benjamingr benjamingr deleted the patch-1 branch June 18, 2015 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants