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

Normative: Make array spread accept nullish values #1069

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@mathiasbynens
Member

mathiasbynens commented Jan 16, 2018

Previously, [42, ...undefined] threw a TypeError exception.

The object spread proposal has the more useful behavior of silently ignoring null and undefined values, e.g. { a: 42, ...undefined } results in { a: 42 } instead of throwing an exception.

This patch applies the same concept to array spread, making [42, ...undefined] result in [42].

Normative: Make array spread accept nullish values
Previously, `[42, ...undefined]` threw a TypeError exception.

The object spread proposal has the more useful behavior of silently
ignoring `null` and `undefined` values, e.g. `{ a: 42, ...undefined }`
results in `{ a: 42 }` instead of throwing an exception.

This patch applies the same concept to array spread, making
`[42, ...undefined]` result in `[42]`.
@michaelficarra

This comment has been minimized.

Member

michaelficarra commented Jan 16, 2018

My immediate reaction here is that this is an error and I would like the error to be caught as close to the point where I would fix it as possible. A user can always spread (something ?? []) if they want to account for possibly nullish values.

@getify

This comment has been minimized.

Contributor

getify commented Jan 16, 2018

I find it quite strange (aka surprisingly inconsistent) that ... would silently swallow errors when used against null'ish values, but that destructuring does not, and requires you to do the = {} / = [] defaults stuff.

If we relaxed the errors and let all of those forms fail silently, at least we'd have some consistency there.

@littledan

This comment has been minimized.

Member

littledan commented Jan 16, 2018

Does anyone remember why Object.assign permitted undefined in the first place, leading to the design of object spread? cc @rwaldron

@anba

This comment has been minimized.

Contributor

anba commented Jan 17, 2018

Does anyone remember why Object.assign permitted undefined in the first place, leading to the design of object spread?

It was changed in ES6 draft rev27 per discussion in the July 29 2014 TC39 meeting.

@littledan

This comment has been minimized.

Member

littledan commented Jan 17, 2018

The key seems to be,

Covered existing libraries to use Object.assign, feedback almost always included the undefined case.

@sebmarkbage Do you have any information from these libraries about why they made that decision?

@mathiasbynens

This comment has been minimized.

Member

mathiasbynens commented Jan 17, 2018

We could also make this change in GetIterator itself so that null and undefined become iterable — but the current PR seemed like a less controversial change.

Let’s discuss this at next week’s meeting! Do we need a separate agenda item, or does this fall under the “needs consensus” PR topic?

@littledan

This comment has been minimized.

Member

littledan commented Jan 17, 2018

@mathiasbynens I've been trying to put needs-consensus PRs that I want to discuss on the agenda in advance, so it's easier to for committee members to look into the issue ahead of time.

@sebmarkbage

This comment has been minimized.

Contributor

sebmarkbage commented Jan 17, 2018

@littledan I suspect that the primary reason this patterned evolved to begin with was because this was how you implemented it:

function assign(target, source) {
  for (var key in source) {
    ... 
  }
}

And since for...in treats null/undefined as if there were no keys, this naturally follows. So I'd say that for..in was the real precedence. Patterns evolved as a consequence.

It turns out to be very useful since it comes up very frequently that you have optional additional properties such as a configuration object.

When I suggested in circles of early spread users that it could be more restrictive than for..in and assign there was a lot of push back because it would lead to a lot of patterns like ...(obj || {}).

Treating it as an empty set of keys is convenient and also not a particularly confusing semantic. What else could it mean?

Sure, it might cover up a mistake but so can many other things. For static type systems it is not really a problem because they'll instead catch it at the resulting type if there's a problem. For Flow specifically it would probably be worse to have the obj || {} since empty object means something special.

The mental model here is that it is not property access. It is key extraction followed by property access. The property access is consistent with how destructuring and defaults work. The key extraction just happens to result in no keys, and therefore there isn't a property access and therefore no error. The same rationale applies to why own/enumerability differ from destructuring.

For arrays, it is a bit different because the equivalent of "key extraction" is extracting the iterator. You could argue that extracting the iterator from null/undefined should be able to be an empty iterator for the same reason that they have an empty set of keys.

@sebmarkbage

This comment has been minimized.

Contributor

sebmarkbage commented Jan 17, 2018

In general, I think that if two patterns evolve, we should choose the most permissive solution that allows both patterns to co-exist instead of trying to kill one.

For object spread, there is another interesting property. All other primitive values (number, string, symbol, boolean) also gets treated as having an empty set of keys since their ToObject forms have no own/enumerable properties.

That is not the case for array spread, since strings have iterators.

@getify

This comment has been minimized.

Contributor

getify commented Jan 18, 2018

What about for..of loops? Those are generally treated as the imperative equivalent of ... spreading (in that they both consume an iterator to completion), so are we also going to relax that form for consistency sake?

@ljharb

This comment has been minimized.

Member

ljharb commented Jan 18, 2018

I would absolutely expect for (x of obj) { foo(x); } to behave identically to [...obj].forEach(x => foo(x)), for any obj - so yes, if we changed array spread, I'd say we'd want to change for..of as well.

@getify

This comment has been minimized.

Contributor

getify commented Jan 18, 2018

What about relaxing the need for || {} in:

let { x, y } = obj || {};

and both of the = {} in:

function foo({
   x: { a, b } = {} 
} = {}) { .. } 

And of course the [ ] array destructuring counterparts.

If the motivation of relaxing ... spreading (object and array) is purely to avoid the tedium of those fallback/default guard clauses, then for consistency we should consider alleviating that tedium for these destructuring forms, too.

FWIW, in my code I have to write far more guard clauses in my destructuring than in my spreading.

It also seems that the concern of "what if I expect/prefer exceptions on null or undefined?" equally applies to ... spreading as it does to destructuring. Which is to say it's probably minor but it's not zero.

@benderTheCrime

This comment has been minimized.

benderTheCrime commented Jan 18, 2018

@getify

This comment has been minimized.

Contributor

getify commented Jan 18, 2018

@benderTheCrime I think one generally accepted exception (pun intended) or caveat to the non-breaking commitment is that it's seen as OK to relax (eliminate) an exception/error where it used to be thrown and now isn't (for any of a variety of reasons). The reason this still qualifies as non-breaking is it's not really guaranteed that exceptions are constant across spec revisions.

Note: The reverse is (usually) not allowed: introducing an exception where none used to be.

@benderTheCrime

This comment has been minimized.

benderTheCrime commented Jan 18, 2018

Hey, sorry @getify, I elaborated on my example a little bit, where an exception is raised/thrown as a direct byproduct of the code, but actually caught by some catch behavior that has been delegated. Is that still considered a non-breaking change regardless of the cases where this function will now behave very differently?

@benderTheCrime

This comment has been minimized.

benderTheCrime commented Jan 18, 2018

@getify

This comment has been minimized.

Contributor

getify commented Jan 18, 2018

@benderTheCrime I think there's a difference between "breaks my code" and "breaks my code in a way we promised it wouldn't break".

There's lots of scenarios that can be created, like the one you suggest, where theoretically someone's code could be broken. But ultimately they would all have been relying on an exception that wasn't necessarily and explicitly guaranteed to always be thrown.

Think of it this way: we guarantee that a thing will always be there (once it's there), but we don't guarantee the inverse: a thing that's not there will never be there.

Moreover, an overriding principle is that no change, whether justifiable with the above reasoning or not, sticks if it turns out that it breaks "enough" existing code that browsers refuse to implement it. The definition of "enough" varies a fair bit between browsers and issues at hand. So ultimately, even if we decided we wanted to do as discussed in this thread, if it turns out that it's actually (not just theoretically) breaking "enough" code to matter, if will be backed out.

@ljharb

This comment has been minimized.

Member

ljharb commented Jan 18, 2018

@benderTheCrime in the code you have, anything with a next() that throws would trigger the catch too; your code isn’t verifying that it’s an array (only Array.isArray can do that), it’s just verifying that arr is an object with an iterator that doesn’t throw. Changing what builtin values are in that set doesn’t break the guarantee your code actually has (only the one you incorrectly believe it has).

@benderTheCrime

This comment has been minimized.

benderTheCrime commented Jan 18, 2018

@ljharb

This comment has been minimized.

Member

ljharb commented Jan 18, 2018

It’s on the agenda to discuss at this month’s meeting.

If you can point at actual non-contrived places in existing code where it would be breaking, that’d be helpful.

@not-an-aardvark

This comment has been minimized.

Contributor

not-an-aardvark commented Jan 20, 2018

For me, the mental model of [...x] is a collection over x[Symbol.iterator](), and the mental model of {...x} is a collection over Object.keys(x).

x[Symbol.iterator]() and Object.keys(x) both throw when given undefined, so I would expect [...x] and {...x} to throw as well.

I don't find the argument about not wanting to use ...(obj || {}) to be very compelling -- it seems designed to prevent developers from needing to think about whether their values are nullable, but in reality they usually need to think about that anyway (since foo.bar throws if foo is undefined).

Changing the behavior of for..of seems particularly worrying due to how easy it is to accidentally end up with an undefined in JavaScript:

for (const foo of myObject.elments /* oops, typo */) {}

Currently, this code will throw an error, making the bug obvious. However, the code would be very difficult to debug if it acted the same as an empty array, because it would be swallowing the error and implicitly replacing it with a seemingly-valid result. I don't think this would be an improvement for developer experience.

@littledan

This comment has been minimized.

Member

littledan commented Jan 20, 2018

@getify 's suggestion here is interesting. Does anyone remember why we decided in ES6 that we shouldn't allow iteration over undefined? @not-an-aardvark 's argument is somewhat compelling, but I can see both sides here. I had trouble finding something in the tc39-notes repository going back to 2012; not sure if this is in earlier notes. cc @allenwb

@mathiasbynens

This comment has been minimized.

Member

mathiasbynens commented Jan 24, 2018

At the January TC39 meeting, there was strong opposition to changing the way object spread works. There was some opposition to changing the way array spread or iteration works due to lack of use cases. Consistency by itself is not enough of an argument.

@michaelficarra

This comment has been minimized.

Member

michaelficarra commented Jan 24, 2018

I present a survey of all internal uses of the iteration protocol and their handling of null/undefined values:

code x = null x = undefined
[...x] TypeError TypeError
let [a] = x TypeError TypeError
[] = x TypeError TypeError
f(...x) TypeError TypeError
for (let a of x); TypeError TypeError
(function*() { yield* x; }().next()) TypeError TypeError
Array.from(x) TypeError TypeError
%TypedArray%.from(x) TypeError TypeError
new %TypedArray%(x) TypeError TypeError
new Map(x) empty Map empty Map
new Set(x) empty Set empty Set
new WeakMap(x) empty WeakMap empty WeakMap
new WeakSet(x) empty WeakSet empty WeakSet
Promise.all(x) rejected promise (TypeError) rejected promise (TypeError)
Promise.race(x) rejected promise (TypeError) rejected promise (TypeError)
@raulsebastianmihaila

This comment has been minimized.

raulsebastianmihaila commented Jan 24, 2018

(function*() { yield* null; }()) does nothing W.R.T. null because the yield statement isn't evaluated. .next() needs to be called and then an error is thrown.

@michaelficarra

This comment has been minimized.

Member

michaelficarra commented Jan 24, 2018

@raulsebastianmihaila Thanks! Updated the table.

@erights

This comment has been minimized.

erights commented Jan 24, 2018

@michaelficarra Please update the rejected promise entries to show the type of error with which the promise is rejected. Thanks.

@michaelficarra

This comment has been minimized.

Member

michaelficarra commented Jan 24, 2018

@getify Object spread isn't built on top of a (exposed) protocol as far as I'm aware. There's really nothing to compare it to.

@michaelficarra

This comment has been minimized.

Member

michaelficarra commented Jan 26, 2018

@mathiasbynens Is it time to close this? We now know that the current behaviour is consistent with every other use of the iteration protocol except constructor parameters, which all behave as if the parameter was not provided.

@jdalton

This comment has been minimized.

Member

jdalton commented Jan 26, 2018

I think spread behaviors (objects and arrays) should be consistent. As others have mentioned, the error case here seems odd given the behavior of Object.assign and object spread. Since there's already a precedent for deviating from the iteration protocol error I think array spread should be another such case.

This should be viewed with the lens of consistency with spreads and not with the lens of consistency with the iteration protocol. I consider the fact that spreading in arrays can error in this way to be a spec bug (so FWIW I'm 👍 on this PR).

If there's no general agreement that this is a spec bug fix, then should it proceed as any other syntax proposal with its own proposal repo, tc39 agenda item, and so on?

@michaelficarra

This comment has been minimized.

Member

michaelficarra commented Jan 26, 2018

@jdalton This was already brought in front of the committee during this week's TC39 meeting and did not achieve consensus because, among other things, consistency with object spread was not a compelling argument. I suggested that there may be changes needed to reach consistency among other uses of the iteration protocol, which is why I did the survey I posted above, but that turns out to not require any changes either.

@jdalton

This comment has been minimized.

Member

jdalton commented Jan 26, 2018

because, among other things, consistency with object spread was not a compelling argument.

It should be compelling enough if TC39 folks are thinking with their dev/user hats on 😞

@mathiasbynens

This comment has been minimized.

Member

mathiasbynens commented Jan 26, 2018

After discussing this at TC39 and especially after perusing @michaelficarra’s table I’m less convinced that we should change anything here.

  1. Making null and undefined iterable would be weird because they lack a prototype on which Symbol.iterator can be available.

  2. While making array spread “ignore” nullish values would make it consistent with object spread, it would make it inconsistent with other entries in @michaelficarra’s table where iterating over nullish values throws. It’s interesting that this doesn’t happen for Map/Set/WeakMap/WeakSet, so maybe we could change all the throwing cases (including array spread) accordingly, but I’m not sure how I feel about that yet.

@gibson042

This comment has been minimized.

Contributor

gibson042 commented Jan 26, 2018

Looking at the table, it seems more reasonable to throw a TypeError when object-spreading null or undefined and let Object.assign be the odd one out (after all, it is standard library instead of syntax). But that proposal just advanced to Stage 4 three days ago, so ¯\_(ツ)_/¯.

@mathiasbynens

This comment has been minimized.

Member

mathiasbynens commented Jan 26, 2018

@gibson042 The committee consensus is that object spread must continue to match Object.assign.

@allenwb

This comment has been minimized.

Member

allenwb commented Jan 26, 2018

It’s interesting that this doesn’t happen for Map/Set/WeakMap/WeakSet, so maybe we could change all the throwing cases (including array spread) accordingly, but I’m not sure how I feel about that yet.

The behavior of those constructors is primarily there to handle to 0 arguments cases like new Set or new Map() which by design are intended to mean create an empty collection. undefined is simply how a missing argument is converted to a parameter value. Null pretty much this falls out of that same process and can also be interpreted for those specific functions as being an explicit way of saying "no iterator provided"

@gibson042

This comment has been minimized.

Contributor

gibson042 commented Jan 27, 2018

That committee consensus is at odds with both the already-specified array spread and apparent developer consensus (not that either are news to you, it's your PR and your survey). JS won't die from one more inconsistency, but it does seem a bit wrong for syntax constructs (as opposed to function calls) to swallow exceptions. I mean, it's not like the in or instanceof operators tolerate invalid right-hand operands.

@ljharb

This comment has been minimized.

Member

ljharb commented Jan 27, 2018

Object spread is intended to match Object.assign exactly, and an inconsistency there would be far far worse than a difference between objects and iterables.

As for syntax, typeof has swallowed ReferenceErrors for years (modulo let/const TDZ errors).

@gibson042

This comment has been minimized.

Contributor

gibson042 commented Jan 27, 2018

Object spread is intended to match Object.assign exactly, and an inconsistency there would be far far worse than a difference between objects and iterables.

Can you elaborate? Data from the community seems to oppose that position (and is at best mixed), and anyway the exact match you desire is already missing:

// throws
let a = Object.assign({ set x(v) { throw v } }, {x: 0});

// copies properties
let b = { set x(v) { throw v }, ...{x: 0} };

As for syntax, typeof has swallowed ReferenceErrors for years (modulo let/const TDZ errors).

typeof does not expect its operand to be an object, unlike instanceof and in and object spread.

@allenwb

This comment has been minimized.

Member

allenwb commented Jan 27, 2018

This discussion seems moot because TC39 has already reached a consensus on the question. But, for the record:

Object spread is intended to match Object.assign exactly, and an inconsistency there would be far far worse than a difference between objects and iterables.

Object.assign's behavior was designed to exactly match what it would be if a for-in loop that skipped inherited properties was used for the property iteration. That choice was made because similar functions in several popular frameworks were implemented using such for-in loops. The assumption was that over time those functions would be replaced by Object.assign or reimplemented using Object.assign and we wanted to ensure compatibility with the previous implementations.

I personally see no reason why compatibility with a single (and quite new) built-in function should have such an impact upon the design of a syntactic operator. If I had been designing the object spread operator consistency with array spread would have driven my decision, not consistency with Object.assign. Consistency among functions: important. Consistency among operators: important. Consistency among operators and functions that break the other important consistencies: mistake.

@ljharb

This comment has been minimized.

Member

ljharb commented Jan 27, 2018

@allenwb Thanks for the clarification.

Regardless, the operator isn't actually "array spread" - it's "iterable spread". Iterability is determined by looking up a Symbol property on an object, which throws on null/undefined; "object spread" doesn't have to do that since there's no property lookup. I don't see this as a mistake nor as an inconsistency.

@getify

This comment has been minimized.

Contributor

getify commented Jan 27, 2018

@ljharb

I don't see this as a mistake nor as an inconsistency.

With all due respect (and as @jdalton opined), that sounds like viewing things through spec-colored glasses rather than through the glasses of typical/general end-user developers.

I interact regularly with such general developers of all skill levels, and I can say with strong certainty that they're not particularly thinking about the details of ... used on arrays as instantiating an iterator but used on an object performing some other unrelated key enumeration algorithm. IME, they tend to assume a somewhat shared/common generic "spreading" of values/entries between the two, and don't really think deeply about how that occurs. For many of them, my teaching experience tells me they're going to see this as an inconsistency.

The TC39 consensus may indeed be that this inconsistency is acceptable (aka, better than alternatives), but I think it's unreasonable to take the position that it's not an inconsistency and rather the fault of general developers not studying deeply enough the internals of the two implementations. The differences between ...x when x is an array vs. an object are far less obvious than you're implying.

There's not usually a need for such developers to go to that level of detail. Such nuances usually only surface to the general end-user as a result of leaky design.

@gibson042

This comment has been minimized.

Contributor

gibson042 commented Jan 27, 2018

Iterability is determined by looking up a Symbol property on an object, which throws on null/undefined; "object spread" doesn't have to do that since there's no property lookup. I don't see this as a mistake nor as an inconsistency.

Object spread doesn't have to lookup @@iterator, but it does rely upon [[OwnPropertyKeys]] and only avoids ToObject exceptions by explicitly special-casing undefined and null. The two kinds of spread syntax are inconsistent, though opinions obviously differ about whether or not that inconsistency is a mistake (and those opinions seem to be irrelevant anyway, since Stage 4 status means that object spread will be in the next revision).

I believe it was, but like I said, JS won't die from one more inconsistency.

@sebmarkbage

This comment has been minimized.

Contributor

sebmarkbage commented Jan 30, 2018

My take away from the committee was that consistency alone wasn't enough of an argument (especially given other consistency issues such as the ones mentioned here and with how other primitives work). It seems like this thread is stuck on the consistency argument.

That's not to say that there can't be other arguments for making this change. Such as that it is useful. It is possible to relax many of the existing errors too if it is useful to do so. I haven't seen explorations into what examples leave you with null/undefined and if the syntactic burden of checking for those is worth it or not, or even how you would do so.

One interesting artifact is that the form [...iterable || []] isn't as obvious as it is in the object form because empty string would turn into an array, but the empty string is a valid iterable. Both of those yield zero elements though so it seems fine that they would be equivalent, but are there other quirks with this pattern?

Are there other inconveniences that would motivate this change?

@jdalton

This comment has been minimized.

Member

jdalton commented Feb 1, 2018

Are there other inconveniences that would motivate this change?

Related, I found myself wrapping for-of use in if statements to avoid the nullish problems.
I'm now using a loose mode Babel plugin to avoid it.

@littledan

This comment has been minimized.

Member

littledan commented Feb 4, 2018

@jdalton Could you point to a case where this came up for you?

EDIT: Oops, I didn't scroll down enough, the example is right there.

@jdalton

This comment has been minimized.

Member

jdalton commented Feb 4, 2018

@littledan

For the for-of case the link is 👆. For the spread nullish errors I happen to have lucked out and not hit an error but I found several spots in my code that could easily bork if the value is nullish: Here, here, here, here, and here. That's enough cases for me to transpile in a loose mode for that too.

@not-an-aardvark

This comment has been minimized.

Contributor

not-an-aardvark commented Feb 4, 2018

Could you elaborate on why you're spreading a value if you're not sure whether it's nullish? For example, it seems like you could also get an error if the value is an empty object rather than an array. It seems like the general solution would be to ensure that you only use array-spread on values which are actually iterable.

@jdalton

This comment has been minimized.

Member

jdalton commented Feb 4, 2018

Could you elaborate on why you're spreading a value if you're not sure whether it's nullish

It's not something I had considered a gotcha. It turns out I also had loose mode enabled 😋 as a blanket setting. If I start a project without transpiling then I'll likely have to throw in if (maybeNull) { checks before all my spreads in addition to for-ofs. For now I'll be transpiling away this bit whenever possible.

@justinfagnani

This comment has been minimized.

justinfagnani commented Mar 12, 2018

At the January TC39 meeting, there was strong opposition to changing the way object spread works. There was some opposition to changing the way array spread or iteration works due to lack of use cases. Consistency by itself is not enough of an argument.

I just his this issue when trying to convert some code that used concat. I was tempted to write:

[...listOne, ...listTwo]

but had to write this instead:

[...listOne || [], ...listTwo || []]

Compared to being able to just write {...ob}, this is very unintuitive. Especially given that Object.keys(null) throws, and object spread doesn't.

If we can't agree to make spread null-safe, can we add a null-safe version of spread as part of the optional-chaining proposal? Maybe [...?listOne] and f(...?args)?

@WebReflection

This comment has been minimized.

WebReflection commented Mar 12, 2018

but had to write this instead: [...listOne || [], ...listTwo || []]

that also assumes one remember precedence of the || over ..., otherwise it would look even uglier:

[...(listOne || []), ...(listTwo || [])]

I agree [...listOne, ...listTwo] is the cleanest way to go/think about it but if ? gets introduced it should consider other use cases where it makes sense too (thinking about maybe=?b.maybe where ? is about the whole path or others)

@wmertens

This comment has been minimized.

wmertens commented Jun 15, 2018

but had to write this instead: [...listOne || [], ...listTwo || []]

that also assumes one remember precedence of the || over ..., otherwise it would look even uglier:

[...(listOne || []), ...(listTwo || [])]

That's how I write it and I dislike it, syntax-wise and performance-wise.

Maybe it gets optimized away, but what happens here is

  1. Check if listOne is falsy
  2. If it is, create a new array
  3. Iterate the empty array into the new array, yielding no elements

Whereas, if the array spread syntax would ignore null and undefined, it becomes the much nicer

  1. do nothing
@mathiasbynens

This comment has been minimized.

Member

mathiasbynens commented Aug 22, 2018

I’m gonna close this, as the suggestion didn’t get committee consensus. Happy to continue the discussion in the closed issue though.

@mathiasbynens mathiasbynens deleted the mathiasbynens:array-spread-nullish branch Aug 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment