Rule: Do not use || for defaulting by adrienverge · Pull Request #590 · airbnb/javascript

adrienverge · 2015-11-24T22:03:41Z

Defaulting omitted variables using || is like using == instead of ===. If a function's argument is falsy (0, false, null, ''...), it will be defaulted while it shouldn't be.

Example:

// bad
function f(arg, optionalArg, opts = {}) {
    const message = optionalArg || 'not set';  // what if arg === ''?
    const rate = opts.rate || 0.1;             // what if arg === 0.0?
}

// bad
const useUnicode = ENV['USE_UTF8'] || true;    // what if arg === false?

// good
function f(arg, optionalArg = 'not set', opts = {}) {
    const rate = opts.rate !== undefined ? opts.rate : 0.1;
}

// good
const useUnicode = ENV['USE_UTF8'] !== undefined : ENV['USE_UTF8'] || true;

More reading: http://www.codereadability.com/javascript-default-parameters-with-or-operator/

ljharb · 2015-11-24T22:05:45Z

It's good to avoid the footgun of a falsy non-undefined value when using || - however, in the case of your example, where "port" needs to be a positive integer, opts.port || 80 is actually more accurate than your "good" example, although nowhere near as precise as it should be.

Separately, is there an eslint rule that would enforce this?

goatslacker · 2015-11-24T22:17:48Z

Does this rule not handle this case?

ljharb · 2015-11-24T22:19:09Z

@goatslacker that talks about reusing function argument names, which deopts in v8 and hurts readability, but this PR seems to be talking about use of || for defaulting any value, even to a new var.

adrienverge · 2015-11-24T22:38:23Z

Thanks for feedback @ljharb @goatslacker.

in the case of your example, where "port" needs to be a positive integer, opts.port || 80 is actually more accurate

That's true, port was not a good a example. I've updated it.

Does this rule not handle this case?

Rule 7.7 mentions that using || is a bad practice, but is about the default parameter syntax. The rule I want to add applies to any var in general (including opts arg: opts.optionalKey).

goatslacker · 2015-11-24T22:43:22Z

README.md

this is a syntax error

My bad, sorry. I'm correcting it.

Defaulting omitted variables using `||` is like using `==` instead of `===`. If a function's argument is falsy (`0`, `false`, `null`, `''`...), it will be defaulted while it shouldn't be. Example: ```javascript // bad function f(arg, optionalArg, opts = {}) { const message = optionalArg || 'not set'; // what if arg === ''? const rate = opts.rate || 0.1; // what if arg === 0.0? } // bad const useUnicode = ENV['USE_UTF8'] || true; // what if arg === false? // good function f(arg, optionalArg = 'not set', opts = {}) { const rate = opts.rate !== undefined ? opts.rate : 0.1; } // good const useUnicode = ENV['USE_UTF8'] !== undefined : ENV['USE_UTF8'] || true; ``` More reading: http://www.codereadability.com/javascript-default-parameters-with-or-operator/

adrienverge · 2015-11-30T08:59:47Z

Separately, is there an eslint rule that would enforce this?

I did not see any rule like that. It would be hard to enforce, since || is legitimate in certains cases (boolean operations).

ljharb · 2015-11-30T21:41:13Z

It seems like an eslint rule could look for assignments where the right hand side contained a ||? There very well may not be an existing rule though.

justjake · 2015-11-30T21:43:51Z

@ljharb then we have to tell if the values involved in the right-hand side are nullable or something -- it is possible to have this sort of check with Flow or some other null-aware type system but I don't think you could write a heuristic in ESLint that did anything but cause sadness.

eg const shouldRun = isOnline() || (isOffline() && isIsomporphic()) should be allowed

ljharb · 2015-11-30T21:46:20Z

@justjake the idea would be to always disallow || in assignments, not just nullable values.

adrienverge · 2015-12-01T10:00:43Z

the idea would be to always disallow || in assignments, not just nullable values.

Actually what I proposed was less strict: disallow || for defaulting, i.e. do not use || for non-boolean values.

eg const shouldRun = isOnline() || (isOffline() && isIsomporphic()) should be allowed

I agree.

ljharb · 2015-12-01T15:27:26Z

@adrienverge that means there's no way we could enforce a lint rule then, because there's no way to tell at compile time that isOffline, for example, is a boolean.

adrienverge · 2015-12-01T15:47:50Z

@ljharb Absolutely. But in my opinion, not being able to enforce it does not mean we shouldn't recommend it.

jonmarkprice · 2016-04-08T04:13:32Z

#188 generalizes this to include &&.

adrienverge force-pushed the rule/do-not-use-or-for-defaulting branch from 1ee18bd to dd90d55 Compare November 24, 2015 22:38

goatslacker reviewed Nov 24, 2015
View reviewed changes

adrienverge force-pushed the rule/do-not-use-or-for-defaulting branch 2 times, most recently from 82aaf0d to 30ec484 Compare November 24, 2015 22:56

adrienverge force-pushed the rule/do-not-use-or-for-defaulting branch from 30ec484 to 81c7c53 Compare November 24, 2015 23:02

ljharb added needs rebase question labels Dec 23, 2015

felipethome mentioned this pull request Aug 4, 2016

[guide] Add a why quote in the 7.7 topic and remove comments #996

Open

odykyi approved these changes Oct 5, 2017

View reviewed changes

Conversation

adrienverge commented Nov 24, 2015

Uh oh!

ljharb commented Nov 24, 2015

Uh oh!

goatslacker commented Nov 24, 2015

Uh oh!

ljharb commented Nov 24, 2015

Uh oh!

adrienverge commented Nov 24, 2015

Uh oh!

goatslacker Nov 24, 2015

Choose a reason for hiding this comment

Uh oh!

adrienverge Nov 24, 2015

Choose a reason for hiding this comment

Uh oh!

adrienverge commented Nov 30, 2015

Uh oh!

ljharb commented Nov 30, 2015

Uh oh!

justjake commented Nov 30, 2015

Uh oh!

ljharb commented Nov 30, 2015

Uh oh!

adrienverge commented Dec 1, 2015

Uh oh!

ljharb commented Dec 1, 2015

Uh oh!

adrienverge commented Dec 1, 2015

Uh oh!

jonmarkprice commented Apr 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants