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

Update for lodash v4. compatibility #308

Merged
merged 2 commits into from
Sep 13, 2016
Merged

Conversation

bazineta
Copy link
Contributor

Modifications to support lodash v4.

  • ‘this’ can’t be passed as the final argument in things like _.each,
    and must instead be bound explicitly via _.bind.
  • ‘invoke’ is now ‘invokeMap’, but underscore lacks ‘invokeMap’. To run
    with both underscore and lodash, convert to an _.each loop.
  • ‘contains’ is now ‘includes’. As underscore has ‘includes’ as an
    alias for ‘contains’, convert to ‘includes’.

Modifications to support lodash v4.
- ‘this’ can’t be passed as the final argument in things like _.each,
and must instead be bound explicitly via _.bind.
- ‘invoke’ is now ‘invokeMap’, but underscore lacks ‘invokeMap’. To run
with both underscore and lodash, convert to an _.each loop.
- ‘contains’ is now ‘includes’. As underscore has ‘includes’ as an
alias for ‘contains’, convert to ‘includes’.
@akre54
Copy link
Contributor

akre54 commented Sep 13, 2016

Ha. I swapped these out a couple years back (#157) because I like the 3rd arg style a bit more.

I don't really like the rest of the lodash changes either as I think they make the code harder to read, but thanks for going through this and cleaning up places.

Finally, I think we're at the point where we can use Function.prototype.bind instead of _.bind. We can also use the old var self = this or var _this = this pattern instead of bind, which I think uglies up the code.


// Call `_destroy` on a unique list of the binding callbacks.
_.each(_.uniq(destroyFns), function(fn) { fn.call(this); }, this);
_.each(_.uniq(destroyFns), _.bind(function(fn) { fn.call(this); }, this));
Copy link
Contributor

Choose a reason for hiding this comment

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

how about just bind here, since that's what this function is doing anyway?

@bazineta
Copy link
Contributor Author

Well, so far as bind vs. _.bind vs self, doesn't much matter to me. However, since you "don't really like the rest of the lodash changes either", that's pretty much a nonstarter, so probably best just to close the PR.

@akre54
Copy link
Contributor

akre54 commented Sep 13, 2016

No I mean I don't much like the changes JDD has made. But if this helps people with compatibility happy to merge in. Wanna tweak the minor fixes and then we can :shipit:

@bazineta
Copy link
Contributor Author

JDD's ways are such a mystery to us all.

So I think this nets out to replacing all the _.bind calls with native bind, which seems to affect only IE8 types, which seems like acceptable losses. All straightforward changes but for the 'destroyFns' loop, which I need to get my head around -- suggestions welcome on that one.

@akre54
Copy link
Contributor

akre54 commented Sep 13, 2016

Sure, that sounds reasonable.

Forget my second point, I got confused :)

@akre54 akre54 merged commit 746a274 into nytimes:master Sep 13, 2016
@bazineta
Copy link
Contributor Author

Changed to use native bind().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants