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

Replace lodash 'escapeRegExp' with escape-string-regexp library #11842

Merged
merged 2 commits into from Jul 20, 2020
Merged

Replace lodash 'escapeRegExp' with escape-string-regexp library #11842

merged 2 commits into from Jul 20, 2020

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Jul 15, 2020

Q                       A
Patch: Bug Fix? No
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? No
Any Dependency Changes? Yes
License MIT

From previous discussion about removing lodash dependencies, it sounds like we can use the widely-used escape-string-regexp library to replace the functionality provided by lodash/escapeRegExp.

As noted in that discussion, this module requires Node 10 or greater, and that won't be guaranteed in babel until version 8, so this changeset is based against the next-8-dev branch.

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 15, 2020

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/25985/

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 15, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 82745ff:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@JLHwung
Copy link
Contributor

JLHwung commented Jul 15, 2020

@jayaddison Updating the yarn.lock should fix the e2e error.

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Note that compared to _.escapeRegExp, escape-string-regexp also escapes - to \\x2d: https://github.com/sindresorhus/escape-string-regexp/blob/b3eb767656fe6dbb8c8dbf31746171cfb7b23b78/index.js#L12

Although unnecessary as this is for PCRE compatibility, I think it is acceptable to replace _.escapeRegExp, which also applies toString on the input -- but we never pass a non-string input to escapeRegExp.

@nicolo-ribaudo
Copy link
Member

Thanks!

@nicolo-ribaudo nicolo-ribaudo merged commit e2b33c2 into babel:next-8-dev Jul 20, 2020
@jayaddison jayaddison deleted the dependencies-8/reduce-lodash-usage-escapeRegExp branch July 20, 2020 20:09
@jayaddison
Copy link
Contributor Author

Eh, sort of a nitpick of myself here, but I realized I should have said 'yes' to the 'dependency changes' question in the pull request description Q&A. I'll edit that in the description now (even though it's post-review and post-commit).

@nicolo-ribaudo
Copy link
Member

Oh thanks! That table is only used by us to add PR labels, which are used in the changelog and by reviewers!

@jayaddison
Copy link
Contributor Author

Good to know, thanks @nicolo-ribaudo!

nicolo-ribaudo pushed a commit that referenced this pull request Sep 1, 2020
* Replace lodash 'escapeRegExp' with escape-string-regexp library

* Update yarn.lock
nicolo-ribaudo pushed a commit that referenced this pull request Oct 2, 2020
* Replace lodash 'escapeRegExp' with escape-string-regexp library

* Update yarn.lock
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 21, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants