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

Prefer default parameters #29

Closed
jamestalmage opened this issue May 19, 2016 · 17 comments · Fixed by #632
Closed

Prefer default parameters #29

jamestalmage opened this issue May 19, 2016 · 17 comments · Fixed by #632
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted

Comments

@jamestalmage
Copy link
Contributor

jamestalmage commented May 19, 2016

Issuehunt badges

Applies only to es6 mode:

Bad:

function (foo) {
  foo = foo || 'bar';
  // ...
}

Good:

function (foo = 'bar') {
  // ...
}

IssueHunt Summary

medusalix medusalix has been rewarded.

Backers (Total: $60.00)

Submitted pull Requests


Tips

@sindresorhus
Copy link
Owner

Too bad I can't use default parameters in reusable modules in the near future...

@jfmengels
Copy link
Contributor

The bad and the good examples are pretty different actually, as default parameters only apply when foo is undefined, and the || operator when foo is falsy.
I agree that default parameters should be used instead of || when suitable, but || does have its use cases.

@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label May 31, 2019
@IssueHuntBot
Copy link

@IssueHunt has funded $60.00 to this issue.


@MrHen
Copy link
Contributor

MrHen commented May 31, 2019

It would be nice if this rule had an "always" or "never" configuration option.

@sindresorhus
Copy link
Owner

@MrHen Why would you want never? I'm genuinely curious.

@MrHen
Copy link
Contributor

MrHen commented May 31, 2019

I've stopped using default parameters entirely because of null causing issues with destructuring.

function foo({ bar = 123 } = {}) {
    console.log(bar);
}

foo(null); // TypeError: Cannot destructure property `bar` of 'undefined' or 'null'.

function foo(foo) {
    const { bar = 123 } = (foo || {});
    console.log(bar);
}

foo(null); // 123

It's also super confusing using defaults with destructuring because there are different ways to do it with subtle differences:

function foo2({ bar } = { bar: 123 }) {
    console.log(bar);
}

foo2(); // 123
foo2({}); // undefined

function foo3({ bar = 123 } = {}) {
    console.log(bar);
}

foo3(); // 123
foo3({}); // 123

function foo4({ bar = 123 } = { bar: 456 }) {
    console.log(bar);
}

foo4(); // 456
foo4({}); // 123

I guess what I really care about is not using destructuring in function parameter defaults. Maybe that should just be a different rule.

@sindresorhus
Copy link
Owner

I've stopped using default parameters entirely because of null causing issues with destructuring.

There's a solution to that: I have completely stopped using null.

It's also super confusing using defaults with destructuring because there are different ways to do it with subtle differences:

I would argue the only correct way to do it is this variant function foo2({ bar } = { bar: 123 }) {.

I guess what I really care about is not using destructuring in function parameter defaults. Maybe that should just be a different rule.

Or maybe enforce only using function foo2({ bar } = { bar: 123 }) {?

@MrHen
Copy link
Contributor

MrHen commented May 31, 2019

There's a solution to that: I have completely stopped using null.

Sure, but from a defensive programming standpoint, I'd still rather not blow up on null.

I would argue the only correct way to do it is this variant function foo2({ bar } = { bar: 123 }) {.

If the intent is to provide a default value for bar, then it would be better to use { bar = 123 } = {}. Think of a standard options block:

function foo1({optionA = true, optionB = true} = {}) {
}
foo1({ optionB: false }); // preserves optionA = true

function foo2({optionA, optionB} = {optionA: true, optionB: true}) {
}
foo2({ optionB: false }); // optionA is now undefined and falsey

Or maybe enforce only using function foo2({ bar } = { bar: 123 }) {?

I guess for the scope of this issue / rule, which of these statements are true / approved?

  • Disallow function (foo) { foo = foo || 'bar'; }
  • Disallow function (foo) { const bar = foo || 'bar'; }
  • Allow function (foo = 'bar') { }
  • Allow function (foo = { bar: 123 }) { }
  • Allow function ({ bar } = { bar: 123 }) { }
  • Allow function ({ bar = 123 } = { bar }) { }
  • Allow function ({ bar = 123 } = { }) { }
  • Allow function (foo) { const { bar } = (foo || {}); }

In the end, I would personally prefer not to mess with default parameters at all. I think the code is more readable and less error prone without them. Thus, having a "never" option to invert the check.

For a real world example, I've recently had to go through a bunch of JS React code fixing onChange handlers to not blow up when an external control sent null back when clearing the value of the field:

                <VirtualizedSelect
                    onChange={({ value } = {}) => {
                        // do stuff
                    }}
                />

A lint rule to catch this would have been handy.

@sindresorhus
Copy link
Owner

sindresorhus commented May 31, 2019

Ugh, I copied the wrong code... I meant to copy function foo3({ bar = 123 } = {}) {. That is what I think is the correct solution. (Will comment on the other things tomorrow)

@MrHen
Copy link
Contributor

MrHen commented May 31, 2019

I just realized that #208 is also open and has a similar discussion.

@MrHen
Copy link
Contributor

MrHen commented Jun 10, 2019

Alright, between the two tickets covering this area, here is my recommendation:

no-default-parameter should

  • Allow function (foo) { foo = foo || 'bar'; }
  • Allow function (foo) { const bar = foo || 'bar'; }
  • Allow function (foo) { const { bar } = (foo || {}); }
  • Disallow function (foo = { bar: 123 }) { }
    with fix to function (foo: { bar = 123 } = {}) { }
  • Disallow function ({ bar } = { bar: 123 }) { }
    with fix to function ({ bar = 123 } = {}) { }
  • Disallow function ({ bar = 123 } = { bar }) { }
    with fix to function ({ bar = 123 } = {}) { }
  • Disallow function (foo = fooDefault) { }
    without fix
  • Allow function (foo = {}) { }
  • Allow function (foo = 'bar') { }
  • Allow function ({ bar = 123 } = { }) { }

prefer-default-parameters should

  • Disallow function (foo) { foo = foo || 'bar'; }
    with fix to function (foo = 'bar') { }
  • Disallow function (foo) { const bar = foo || 'bar'; }
    with fix to function (bar = 'bar') { }
  • Disallow function (foo) { const { bar } = (foo || {}); }
    with fix to function ({ bar } = {}) { }
  • Allow function (foo = { bar: 123 }) { }
  • Allow function ({ bar } = { bar: 123 }) { }
  • Allow function ({ bar = 123 } = { bar }) { }
  • Allow function (foo = fooDefault) { }
  • Allow function (foo = {}) { }
  • Allow function (foo = 'bar') { }
  • Allow function ({ bar = 123 } = { }) { }

That being said, I prefer the exact opposite of both of these rules so maybe I'm not the best person to make a call on this.

@JLHwung
Copy link
Contributor

JLHwung commented Jun 17, 2019

The prefer-default-parameters fix would be tricky in some cases since the default parameter is evaluated in the implicit intermediate scope. Consider the following example

let a = "outside";
function f(b) {
  let a = "inside";
  b = b || a;
  return b;
}
f() // inside

The fixed code

let a = "outside";
function f(b = a) {
  let a = "inside";
  return b;
}
f() // outside

would change the semantics and print "outside" when evaluating f() since a is evaluated in parameter scope.

I suggest we limit the rule and fixer capacity that we only fix those with literal as initializer. For those expression initializer, we should not report prefer-default-parameters error and of course, not try to fix that.

@sindresorhus
Copy link
Owner

Sure, but from a defensive programming standpoint, I'd still rather not blow up on null.

I would argue blowing up on null is the most defensive thing you can do. I think you meant to say "graceful", which is ok too, I just personally dread null.


no-default-parameter

I don't really want this. Default parameters are not going away. I don't think it makes sense to fight it.

I would prefer the rule to be named prefer-default-parameter and only enforce default parameters.


prefer-default-parameters should

👍 I agree with the Allow/Disallow's here.

@sindresorhus
Copy link
Owner

I suggest we limit the rule and fixer capacity that we only fix those with literal as initializer. For those expression initializer, we should not report prefer-default-parameters error and of course, not try to fix that.

I agree with not fixing such cases, but why can we not report? The user could still rework it to work as a default parameter.

@MrHen
Copy link
Contributor

MrHen commented Jul 5, 2019

I don't really want this. Default parameters are not going away. I don't think it makes sense to fight it.

Should we close the other issue then? Personally, I would use no-default-parameter over prefer-default-parameters but we obviously can't have both rules default to error. In terms of priority, I think I agree with you that prefer-default-parameters is a better fit for eslint-plugin-unicorn.

@fregante
Copy link
Collaborator

fregante commented Jan 21, 2020

@MrHen this doesn't seem to be valid ES:

function fn (foo: { bar = 123 } = {}) { }
// SyntaxError: missing ) after formal parameters

@issuehunt-oss
Copy link

issuehunt-oss bot commented Dec 20, 2020

@sindresorhus has rewarded $54.00 to @medusalix. See it on IssueHunt

  • 💰 Total deposit: $60.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $6.00

@issuehunt-oss issuehunt-oss bot added 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt and removed 💵 Funded on Issuehunt This issue has been funded on Issuehunt labels Dec 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants