tc39 / proposal-nullish-coalescing

Nullish coalescing proposal x ?? y
https://tc39.github.io/proposal-nullish-coalescing/
1.23k stars 23 forks source link

why modify the language when a trivial helper function returnNotNullishOrDefault() can suffice? #14

Closed kaizhu256 closed 6 years ago

kaizhu256 commented 7 years ago

real-world example of isNullOrUndefined() @ https://github.com/kaizhu256/node-swgg/blob/2017.7.24/lib.swgg.js#L2577

this feature seems redundant if isNullOrUndefined is combined with Optional-chaining-operator (https://tc39.github.io/proposal-optional-chaining/). i have concerns this proposal will make javascript look more like perl and further unreadable.

local.isNullOrUndefined = function (arg) {
/*
 * this function will test if the arg is null or undefined
 */
    return arg === null || arg === undefined;
};
...
// init default param
if (local.isNullOrUndefined(request.swgg.paramDict[paramDef.name]) &&
        paramDef.default !== undefined) {
    request.swgg.paramDict[paramDef.name] =
        local.jsonCopy(paramDef.default);
}
...
ljharb commented 7 years ago

In your example, you have to repeat request.swff.paramDict[paramDef.name] twice. You could of course do:

const obj = a.b.c;
const value = obj == null ? obj : obj.d;

but const value = a.b.c?.d; is much cleaner, more concise, harder to screw up, requires one less variable binding, and the RHS can be a single expression without repetition.

kaizhu256 commented 7 years ago

sorry for misunderstanding (its partly my fault). but after some thinking, i'm raising this issue against ?? (not ?.) and changing the title to reflect new helper function name.

instead of ??, there exist alternative solutions using builtins that don't require the "nuclear" option of language-spec changes.

// instead of
const undefinedValue = response.settings?.undefinedValue ?? 'some other default'; // result: 'some other default'
const nullValue = response.settings?.nullValue ?? 'some other default'; // result: 'some other default'
const headerText = response.settings?.headerText ?? 'Hello, world!'; // result: ''
const animationDuration = response.settings?.animationDuration ?? 300; // result: 0
const showSplashScreen = response.settings?.showSplashScreen ?? true; // result: false

// how about
Object.returnNotNullishOrDefault(value, default) = function {
/ *
  * this function will return value if its null-coalescing, otherwise default
  * (i know Object.returnNotNullishOrDefault is an ugly name for a builtin
  * maybe u guys can think of a better one)
  */
    return value === null || value ==== undefined
        ? value
        : default;
};
const undefinedValue = Object.returnNotNullishOrDefault(
    response.settings?.undefinedValue,
    'some other default'
); // result: 'some other default'
const nullValue = Object.returnNotNullishOrDefault(
    response.settings?.nullValue,
    'some other default'
); // result: 'some other default'
const headerText = Object.returnNotNullishOrDefault(
    response.settings?.headerText,
    'Hello, world!'
); // result: ''
const animationDuration = Object.returnNotNullishOrDefault(
    response.settings?.animationDuration,
    300
); // result: 0
const showSplashScreen = Object.returnNotNullishOrDefault(
    response.settings?.showSplashScreen,
    true
); // result: false
mk-pmb commented 7 years ago

How would you chain the Object.returnNotNullishOrDefault to find the first acceptable value from 5 possible expressions? How will it avoid calculating the 4th and 5th expression if the 3rd expression's result is acceptable?

jridgewell commented 7 years ago

@kaizhu256: Just imagine the default value is some expensive call expression:

const value = response.settings?.value ?? superDuperExpensive();

You'd want to avoid eagerly evaluating the right if the left is acceptable. The only way to do that with your proposal is with another closure function.

ljharb commented 7 years ago

@kaizhu256 ah, my mistake.

Regarding ??, the use case I'm most interested in is in React, where a || b is a footgun if a is zero - a ?? b is easy to understand, to lint for, and solves the use case.

Separately though, I think you're starting from a flawed premise, which is that language spec changes - whether it's new syntax or otherwise - is a "nuclear" option, or even a bad thing. New things are good, evolving the language is good, and while any addition needs to demonstrate that it's an improvement, the mere fact that it's an addition is not a point against it.

kaizhu256 commented 7 years ago

@mk-pmb what you're asking sounds out-of-scope of the current ?? proposal which behaves similarly to || (can you give example code?).

@jridgewell fair enough. the most expensive default fallback i've encountered in real-world would be guaranteeing unique-id for blocking in-memory database. don't have decent counter-argument at moment.

@ljharb - agree to disagree. language-changes disrupt code-coverage tools i use, which makes removing dead-code and cleaning up messes more difficult for me. that's "nuclear" in my book. people who rely on code-coverage to keep web-projects sane and manageable prioritize a stable language-syntax.

also let me present a problematic object that's fairly commonly used (and causes me lots of headaches when getting / setting default values) - process.env. nodejs auto-coerces all assignments to it as a string.

process.env.EXIT_CODE = 0;  // coerced to string "0"
process.env.EXIT_CODE = process.env.EXIT_CODE || 1; // result "0"
process.env.MODE_DEBUG = null // coerced to string "null"
process.env.MODE_DEBUG = process.env.MODE_DEBUG ?? "development" // result  "null"
require('child_process').spawn('foo', [], { env: process.env });  // external-program foo relies on EXIT_CODE and MODE_DEBUG
...
ljharb commented 7 years ago

@kaizhu256 code coverage tools use a parser; parsers should be keeping up to date with the language anyways. Regardless, nobody is forcing you to use new language features - if your tooling breaks on it, then obviously nobody can use it, upgrade to a dependency that uses it, or merge it to master or deploy it (since if your tools aren't gating merges/deploys, you've got way bigger problems than "new syntax is added sometimes"). In other words, you're literally describing a non-problem.

mk-pmb commented 7 years ago

what you're asking sounds out-of-scope of the current ?? proposal which behaves similarly to ||

For me it seems to fit perfectly: Let's use 5 very simple expressions that are just some stub functions named a…e:

function a() { return null; }
function b() { return undefined; }
function c() { return 1; }
function d() { throw new Error('oh no!'); }
function e() { return false; }

let v = ( a() || b() || c() || d() || e() );
let w = ( a() ?? b() ?? c() ?? d() ?? e() );

In that case both expressions should do the same, v === w === 1. How would I express the latter with Object.returnNotNullishOrDefault?

mk-pmb commented 7 years ago

Also I don't see how automatic conversion for process.env , or some setter/getter on another object, is related to the ?? operator. For any case where the string conversion doesn't yield the empty string, ?? should behave the exact same as ||.

kaizhu256 commented 7 years ago

@mk-pmb ahh, i see your point. but do you really want to someday end up maintaining someone-else's code looking like this:

/*
 * something went wrong, where is the bug in the code?
 * is the bug even here or somewhere else?
 * seriously, my eyes are melting trying to figure this out.
 */
var xx = aa() || bb() ?? cc();
var yy = dd() ?? ee() ?? ff();
var zz = gg() || hh();
ljharb commented 7 years ago

Yes, many people do. Regardless, that's a subjective concern - "it's new and unfamiliar" isn't an automatic reason not to do something, and in this case it's not a commonly held opinion.

mk-pmb commented 7 years ago

@kaizhu256, I'll evade the question because it is besides the point. The ability to write obfuscated code isn't much improved with the new operator. The ability to write cleaner code, however, is improved in lots of situations.