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

Add _.restParam #2140

Merged
merged 7 commits into from
Apr 9, 2015
Merged

Add _.restParam #2140

merged 7 commits into from
Apr 9, 2015

Conversation

jridgewell
Copy link
Collaborator

_.restParam is the equivalent of ES6's rest parameter. It's particularly useful as an alternative to slice.call(arguments, index) (numerous examples in this PR), and it's a reusable, very fast version of inline for-loop slicing, eg. jashkenas/backbone#3489.

var test = function(a, b, ...args) {
   console.log(arguments);
}

test(1, 2, 3, 4, 5); // => [1, 2, [3, 4, 5]]

var test = _.restParam(function(a, b, args) {
   console.log(arguments);
});

test(1, 2, 3, 4, 5); // => [1, 2, [3, 4, 5]]

@jridgewell jridgewell force-pushed the restParams branch 3 times, most recently from 252be49 to 0e551d8 Compare April 3, 2015 03:34
@megawac
Copy link
Collaborator

megawac commented Apr 3, 2015

This is heavily inspired by jashkenas/backbone#3489 - related jsperf http://jsperf.com/trigger-with-for-loop-vs-slice/8
Also an awesome pattern it turns out :)

@jashkenas
Copy link
Owner

Super nifty.

How about naming it _.arguments or _.varArgs instead? It is the "arguments" object, after all.

@megawac
Copy link
Collaborator

megawac commented Apr 8, 2015

@jashkenas the name is based off of ES6 rest parameters - we debated the
name pretty heavily over the past couple months before submitting this

Also arguments is ~ a reserved word
http://ecma-international.org/ecma-262/5.1/#sec-12.2.1

On Wed, Apr 8, 2015 at 12:37 PM, Jeremy Ashkenas notifications@github.com
wrote:

Super nifty.

How about naming it _.arguments or _.varArgs instead. It is the
"arguments" object, after all.


Reply to this email directly or view it on GitHub
#2140 (comment).

@jashkenas
Copy link
Owner

The "parameter/argument" naming dichotomy would be unfortunate to introduce — Underscore has historically called them "arguments".

How about _.restArgs?

megawac and others added 6 commits April 8, 2015 18:19
ping @jridgewell @buzzdecafe @davidchambers @jdalton
Implementation proves to be uglier than I had hoped :z going to write some benchmarks after we head for lunch
Fix error

This is the last time I write things in Github editor

Tricky slice

Allow startIndex param

really, really fix it this time

Don't redeclare
@jridgewell
Copy link
Collaborator Author

I'm not against _.restArgs, though I think _.restParam is a much better name since it mirrors ES6's rest parameter.

Also, updated to it merges nicely.

@jashkenas
Copy link
Owner

"parameters" and "arguments" are synonyms for the purposes of JavaScript. "Arguments" is already the established word choice — both for JavaScript with the arguments object, and for Underscore with the _.isArguments function. _.restArgs would be a more consistent name.

@jridgewell
Copy link
Collaborator Author

Sigh, updated.

@jashkenas
Copy link
Owner

Groovy. No way to combine those two similar loops, eh?

jashkenas added a commit that referenced this pull request Apr 9, 2015
@jashkenas jashkenas merged commit 4f2474c into master Apr 9, 2015
@jashkenas jashkenas added the fixed label Apr 9, 2015
@megawac
Copy link
Collaborator

megawac commented Apr 9, 2015

Groovy. No way to combine those two similar loops, eh?

Not without perf JIT penalties (see the initial 2 commits) megawac#3

@megawac megawac deleted the restParams branch April 9, 2015 15:50
@jridgewell
Copy link
Collaborator Author

Yah, there's a pretty bad performance hit, which would defeat the goal of adding this to Backbone's #trigger.

@michaelficarra
Copy link
Collaborator

_.rest wouldn't have worked?

edit: Crap, _.tail is aliased as _.rest...

@jashkenas
Copy link
Owner

_.rest would have been great, but alas:

http://underscorejs.org/#rest

// Similar to ES6's rest param (http://ariya.ofilabs.com/2013/03/es6-and-rest-parameter.html)
// This accumulates the arguments passed into an array, after a given index.
var restArgs = function(func, startIndex) {
startIndex = startIndex == null ? func.length - 1 : +startIndex;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using function.length means functions bound with the _.bind fallback will work inconsistently with ones bound with Function.prototype.bind btw. Is this acceptable? @megawac @jridgewell

var func = function(a,b,c,d,e,f,g) {};

func.length; // 7
func.bind().length; // 7

Function.prototype.bind = null;
_.bind(func).length; // 0

(edit: remove bind(null) for clarity)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the usefulness of not having to give the startIndex every times outweighs this trip-up. A lot of what I imagine using it for is in parse-time. You would then bind the returned function.

var args = _.restArgs(function(args) { return args; }).bind(null);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok cool. You could still pass startIndex to restArgs if you needed the fallback behavior, that seems reasonable.

_.prototype[name] = function() {
var args = [this._wrapped];
push.apply(args, arguments);
_.prototype[name] = restArgs(function(args) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this faster? It's certainly not clearer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It actually looks like it's slower. @megawac?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seemed faster when I profiled it locally on node - I'll check again this weekend

@akre54
Copy link
Collaborator

akre54 commented Apr 10, 2015

I'm 👎 on this change. It may be ugly but slice.call(arguments, 1) is idiomatic JS. It's not yet another underscore-specific helper function that makes code harder to read and reason about.

I'd also argue all of these extra helper methods make Underscore itself harder to read. I really wish we'd simplify a lot of these.

@jashkenas
Copy link
Owner

I agree that this one is a little bit weird — but it's a neat pattern to automatically capture varargs, and to avoid dealing with the arguments object. I think it's worth having.

@jridgewell
Copy link
Collaborator Author

I'm 👎 on this change. It may be ugly but slice.call(arguments, 1) is idiomatic JS. It's not yet another underscore-specific helper function that makes code harder to read and reason about.

The point of this was to get us closer to idiomatic ES6, and provide a speed boost since slice.call(arguments) murders performance.

@akre54
Copy link
Collaborator

akre54 commented Apr 10, 2015

That's great, I just really don't like the "lodashification of Underscore" we've been seeing recently -- helper methods everywhere and a cargo-cult-like focus on microbenchmarks and minor speed improvements at the expense of readability. arguments de-opt fixes are fine, but with this change on any given method you have to read several levels of helpers down to actually understand what it's doing.

One of the best things about Underscore of days past was that it looked like code you might actually write in your project, and in a pinch you could pull out just the parts you needed. Lodash already exists as a separate approach, I'm not sure we need to be copying them here.

@megawac
Copy link
Collaborator

megawac commented Apr 10, 2015

One of the best things about Underscore of days past was that it looked like code you might actually write in your project, and in a pinch you could pull out just the parts you needed. Lodash already exists as a separate approach, I'm not sure we need to be copying them here.

I agree with you and thats why I've avoided several refactors in the past. Me and @jridgewell have just been refactoring in the places which will gain maximum benefit for the library in terms of size and speed. The purpose of a library such as underscore/lodash should be to help devs write quality code

Also a distinction is that nearly all of the helpers used internally are exposed and documented in the API. I'd say its fine for a library to eat its own dog food like this

@akre54
Copy link
Collaborator

akre54 commented Apr 10, 2015

murders performance

Is this the jsperf you're talking about? Sure twice as fast is always awesome, but when you need to trigger 400,000 events in a second, I highly doubt the trigger method (or its args de-opt) is going to be your bottleneck.

its fine for a library to eat its own dog food like this

Sure. I'm more referring to the internal createX methods (reduce comes to mind as particularly egregious) that make Underscore's source nearly impossible to understand if you aren't already intimately familiar. These seem more like exercises to prove we can than things that actually help consumers of the library with any real problem.

@jashkenas
Copy link
Owner

Sure. I'm more referring to the internal createX methods (reduce comes to mind as particularly egregious) that make Underscore's source nearly impossible to understand if you aren't already intimately familiar.

Those are indeed much more problematic. So perhaps this particular issue isn't the right place to make this argument?

It would be great to look at a PR that conceptually simplified the create* mega-methods...

@akre54
Copy link
Collaborator

akre54 commented Apr 10, 2015

So perhaps this particular issue isn't the right place to make this argument?

Totally. This was kinda the straw that broke the camel's back. I wanted to register that I see this change as part of a larger more unfortunate trend. A PR or discussion of how to fix this would be excellent.

@jridgewell
Copy link
Collaborator Author

@akre54 Mind creating it with your thoughts?

@akre54
Copy link
Collaborator

akre54 commented Apr 10, 2015

Will do when I get some time. Work business and non work busyness and all
that. It's been on my to do list

@jridgewell
Copy link
Collaborator Author

👍

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.

None yet

5 participants