proposal-record-tuple icon indicating copy to clipboard operation
proposal-record-tuple copied to clipboard

brad4d Stage 3 Review feedback

Open brad4d opened this issue 2 years ago • 9 comments

For all of these questions I searched through the issues (including closed) and the main-page README.md to find an answer before asking here. If one of these answers is there somewhere, perhaps it could be made more prominent.

  • [ ] 14.2.1 - Why allow class A extends Tuple { at all?
  • [ ] 14.2.3.7 - Why does Tuple.prototype.concat(...args) close up "holes" within the argument array-likes instead of filling in undefined values? This seems inconsistent with the behavior of Array.prototype.concat(...args).

I apologize for leaving this so late. At least I didn't find much to comment on.

brad4d avatar Nov 15 '22 01:11 brad4d

How would one disallow extension? Everything that ES provides can be a superclass, afaik.

ljharb avatar Nov 15 '22 01:11 ljharb

@ljharb It seems to me that there are other globals that cause an exception if you try to use them in an extends clause.

For example:

C = class extends parseFloat {};

Generates this exception in Chrome:

VM287:1 Uncaught TypeError: Class extends value function parseFloat() { [native code] } is not a constructor or null
    at <anonymous>:1:19

I haven't yet tried to find which part of the spec specifies this behavior, but perhaps you already know?

brad4d avatar Nov 15 '22 20:11 brad4d

Ah, yes - functions without a .prototype. However, that wouldn’t make sense when there can be instances of both.

ljharb avatar Nov 15 '22 20:11 ljharb

Why allow class A extends Tuple { at all?

One argument is consistency with other recently added types, extends BigInt is allowed in the same way. Hard to say when to start a new pattern vs follow existing conventions.

acutmore avatar Nov 15 '22 21:11 acutmore

I don't see a strong reason to push back on the extension bullet point. It would be nice to simply refuse to accept Record or Tuple in the extends slot, but I don't think its crucial. The exception throw by calling super() is likely sufficient discouragement.

What about the concat() behavior bullet point, though? Was this deviation from Array.prototype.concat() intentional? Did I miss documentation for why this choice was made?

brad4d avatar Nov 15 '22 21:11 brad4d

Did you mean that #[].concat({ 0: "a", 2: "b", length: 3 }) currently produces #["a", "b"] instead of #["a", undefined, "b"]? If yes, it's a spec bug.

nicolo-ribaudo avatar Nov 15 '22 23:11 nicolo-ribaudo

@nicolo-ribaudo Yes, that's what I mean.

https://tc39.es/proposal-record-tuple/#sec-tuple.prototype.concat

See the loop in step 5.c.v.

If an expected numeric index does not exist, nothing is appended to the tuple. I believe it should be appending an undefined value for that case.

brad4d avatar Nov 16 '22 00:11 brad4d

Regarding extension, new class extends Symbol { constructor(desc) { return arguments.length > 0 ? Object(Symbol(desc)) : Object(Symbol()); } } works, and i'd expect a similar approach to work for Record and Tuple for consistency.

ljharb avatar Nov 16 '22 06:11 ljharb

Edit: I just learned that "The … constructor … is not intended to be used with the new operator or to be subclassed." For others who may not understand the reason why.

I do agree with @ljharb, sometimes consistency trumps reason. This should work, as it does in the playground:

new class extends Record { constructor(obj = {}) { return Object(Record(obj)); } }

joakim avatar Nov 28 '22 19:11 joakim