airbnb
Rule: Do not use || for defaulting
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/
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 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.
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).
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).
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.
@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
@justjake the idea would be to always disallow || in assignments, not just nullable values.
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.
@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.
@ljharb Absolutely. But in my opinion, not being able to enforce it does not mean we shouldn't recommend it.
