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

JavaScript: Avoid referring to arguments #371

Closed
wants to merge 2 commits into from

Conversation

seanpdoyle
Copy link
Contributor

Avoid using arguments unnecessarily.

Inspired by petkaantonov/bluebird's "Optimization Killers"
wiki.

Using arguments without .length or [i] makes it nearly impossible
for V8 to optimize the function block.

*Avoid* using `arguments` unnecessarily.

Inspired by [petkaantonov/bluebird][wiki]'s "Optimization Killers"
wiki.

Using `arguments` without `.length` or `[i]` makes it nearly impossible
for V8 to optimize the function block.

[wiki]: https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#3-managing-arguments
@BlakeWilliams
Copy link
Contributor

👍

2 similar comments
@joshuaclayton
Copy link
Contributor

👍

@drapergeek
Copy link
Contributor

👍

@mcmire
Copy link

mcmire commented Nov 10, 2015

Yup, makes sense. (And you don't need this anymore with the argument spread syntax.)

@seanpdoyle
Copy link
Contributor Author

@mcmire avoiding ...arguments is what this is addressing

@mcmire
Copy link

mcmire commented Nov 10, 2015

Oh, I thought this was addressing avoiding referring to arguments, which is a "special variable" automatically available inside any function.

AFAIK, the spread syntax actually creates a new variable that is non-special. (You could write ...arguments or ...args or ...whatever, in other words.) Or I am off-base on that?

@seanpdoyle
Copy link
Contributor Author

@mcmire I'm not sure. @BlakeWilliams made that comment that inspired this PR, so maybe he know for certain.

@BlakeWilliams
Copy link
Contributor

This example shows that it's still using arguments directly.

@mcmire
Copy link

mcmire commented Nov 10, 2015

Ah, good to know, thanks.

It seems like the following syntax is prohibited in strict mode, as well:

function foo(...arguments) {
  // ...
}

However, you can say this:

function foo(...args) {
  // ...
}

So, do you think that this is useful to distinguish? i.e. something like "As an alternative to arguments, use the spread syntax with a variable that is not arguments."

@BlakeWilliams
Copy link
Contributor

I think it's worth mentioning an alternative any time we recommend against something. 👍 to ...args

@seanpdoyle
Copy link
Contributor Author

@mcmire @BlakeWilliams I've pushed an update with a sample.

@mcmire
Copy link

mcmire commented Nov 10, 2015

Sweet, looks good!

@BlakeWilliams
Copy link
Contributor

Looks great!

@seanpdoyle
Copy link
Contributor Author

During this deep dive into Ember.View, @rwjblue replaces this._super.apply(this, arguments) with this._super(...arguments).

EDITED

@rwjblue
Copy link

rwjblue commented Nov 11, 2015

Technically, I replaced this._super.apply(this, arguments) with this._super(...arguments). The later transpiles to the former (as pointed out earlier in the thread above).

Also, the article referenced specifically states that calling fn.apply is a special exemption from the deopts associated with the arguments object.

@seanpdoyle
Copy link
Contributor Author

@BlakeWilliams from the wiki:

STRICTLY fn.apply(y, arguments) is ok, nothing else is, e.g. .slice. Function#apply is special.

From your example:

_get(Object.getPrototypeOf(Bar.prototype), "constructor", this).apply(this, arguments);

@seanpdoyle
Copy link
Contributor Author

Given this new information, I'm closing this PR.

@seanpdoyle seanpdoyle closed this Nov 12, 2015
@seanpdoyle seanpdoyle deleted the sd-avoid-arguments branch November 12, 2015 00:18
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.

None yet

6 participants