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

Avoid reassign parameters #818

Closed
foisonocean opened this issue Mar 10, 2017 · 8 comments

Comments

@foisonocean
Copy link

commented Mar 10, 2017

I think it's very useful: http://eslint.org/docs/rules/no-param-reassign.html

@dcousens

This comment has been minimized.

Copy link
Member

commented Mar 10, 2017

Ecosystem breakage isn't going to be pleasant.
And honestly, you could just as easily encourage people not to use the arguments object except in limited circumstances.

@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2017

@feross

This comment has been minimized.

Copy link
Member

commented Mar 10, 2017

@yoshuawuyts is right that foo = foo || {} is extremely common. Or even:

function foo (opts) {
  opts = Object.assign({
    default1: true,
    default2: 'cool string'
  }, opts)
}

Also, using arguments isn't really a good idea when we have ...args now:

function foo (...args) {
}

I'd prefer to pass on this rule, even though I agree at a high-level that it sounds desirable. It's just not practical in the real-world.

@feross feross closed this Mar 10, 2017

@dcousens

This comment has been minimized.

Copy link
Member

commented Mar 11, 2017

@feross I think if you could avoid arguments usage, generally, that would be good.
Its only truly useful in an .apply like context no?

@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2017

@dcousens yeah I think so, although it's very common - just in the last week I've written 2 new modules built entirely around this. In the example below there's no escaping using arguments:

// written just for show, idk there might be bugs but yeah.
Emitter.prototype.emit = function (eventName) {
  var event = null
  var args = []
  for (var i = arguments.length - 1; i >= 1; --i) args[i] = arguments[i]
  var events = this._events[eventName]
  if (!events) return
  for (var j = events.length - 1; i >= 0; --i) {
    event = events[j]
    event.apply(event, args)
  }
}

Also, using arguments isn't really a good idea when we have ...args now

Like with most things there's probably fancy ways to write this using new language additions, but I'd rather we allow people (like me ) to use standard without forcing them to tail new specs. I mean: rest params doesn't even have widespread support yet.

@feross

This comment has been minimized.

Copy link
Member

commented Mar 13, 2017

Like with most things there's probably fancy ways to write this using new language additions, but I'd rather we allow people (like me ) to use standard without forcing them to tail new specs

Sure thing. Wasn't trying to enforce that on folks. I was just saying that I'm not too worried about arguments implicitly getting modified when we have rest params as a great up-and-coming alternative.

Also, most functions that use arguments don't use named arguments. It's usually either-or.

@feross

This comment has been minimized.

Copy link
Member

commented Mar 13, 2017

One last thing: rest params have more support than you think! Check it out!

screen shot 2017-03-13 at 1 08 26 pm

We're rapidly approaching a world where you can use the new fanciness and forgo preprocessor complexity like babel. Can't wait.

@dcousens

This comment has been minimized.

Copy link
Member

commented Mar 13, 2017

Also, most functions that use arguments don't use named arguments. It's usually either-or.

Maybe that is something we could look into enforcing?

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
4 participants
You can’t perform that action at this time.