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

Remove uses of lodash/isRegExp #5180

Closed
wants to merge 1 commit into from
Closed

Remove uses of lodash/isRegExp #5180

wants to merge 1 commit into from

Conversation

zertosh
Copy link
Member

@zertosh zertosh commented Jan 21, 2017

Continuing the gradual reduction in lodash use. This is not exactly like lodash's implementation. In Node it defers to util.isRegExp, which is cross-realm safe. In Browsers it does a Object.prototype.toString.call(value) === '[object RegExp]'. I'm fine with switching to the toString approach.

Q A
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Deprecations?
Spec Compliancy?
Tests Added/Pass?
Fixed Tickets
License MIT
Doc PR
Dependency Changes X

@mention-bot
Copy link

@zertosh, thanks for your PR! By analyzing the history of the files in this pull request, we identified @forivall, @hzoo and @boopathi to be potential reviewers.

@loganfsmyth
Copy link
Member

I'm curious, is there a specific goal in mind? I was for the others because I usually prefer natives if it is easy, but this case is less obvious because of the realm issues, and manually writing toString is kind of gross.

@zertosh
Copy link
Member Author

zertosh commented Jan 21, 2017

I think all of lo-dash can actually be removed (only clone and merge are tricky)- saving a non-trivial amount of load time.

@codecov-io
Copy link

codecov-io commented Jan 21, 2017

Current coverage is 89.22% (diff: 100%)

Merging #5180 into master will increase coverage by <.01%

@@             master      #5180   diff @@
==========================================
  Files           203        203          
  Lines          9835       9839     +4   
  Methods        1071       1073     +2   
  Messages          0          0          
  Branches       2620       2620          
==========================================
+ Hits           8775       8779     +4   
  Misses         1060       1060          
  Partials          0          0          

Powered by Codecov. Last update d76092b...485a522

@xtuc xtuc added the PR: Internal 🏠 A type of pull request used for our changelog categories label Jan 21, 2017
@xtuc
Copy link
Member

xtuc commented Jan 21, 2017

Underscore uses:

// Is the given value a regular expression?
  _.isRegExp = function(obj) {
    return !!(obj && obj.test && obj.exec && (obj.ignoreCase || obj.ignoreCase === false));
  };

I think checking for test and exec is safer but could match with other objects.

@zertosh
Copy link
Member Author

zertosh commented Jan 21, 2017

After thinking about it some more, I think the toString (although ugly), is the most effective and honest. Since it's only in 2 places, I don't think it's worth dwelling on the esthetics. Thoughts?

@@ -65,7 +64,7 @@ export function regexify(val: any): RegExp {
return new RegExp(regex.source.slice(1, -1), "i");
}

if (isRegExp(val)) {
if (_isRegExp(val)) {
Copy link
Member

@xtuc xtuc Jan 22, 2017

Choose a reason for hiding this comment

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

If think if we use a function there, we could use the lodash function.

@zertosh
Copy link
Member Author

zertosh commented Jan 22, 2017

I'll leave this change for last. It'll be clearer that keeping _.isRegExp makes no sense when it's the last lodash function used.

@zertosh zertosh closed this Jan 22, 2017
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
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: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants