Spec review #35
Spec review #35
Conversation
| 1. Perform ! DefinePropertyOrThrow ( _O_, `"errors"`, _errorsDesc_ ). | ||
| 1. Let O be ? OrdinaryCreateFromConstructor(_newTarget_, `"%AggregateError.prototype%"`, « [[ErrorData]] »). | ||
| 1. Let _errorsArray_ be ! CreateArrayFromList(? IterableToList(_errors_)). | ||
| 1. Perform ! CreateMethodProperty(_O_, `"errors"`, _errorsArray_). |
There was a problem hiding this comment.
Sure, CreateMethodProperty makes sense here, but, since it's not a method and since this operation is not actively used in the spec, maybe it should be renamed to something like CreateNonEnumerableProperty?
There was a problem hiding this comment.
Perhaps so, but that’s an editorial change I’d make separately.
| </tr> | ||
| </tbody> | ||
| </table> | ||
| </emu-table> |
There was a problem hiding this comment.
AFAIU this table should also include AggregateError.prototype
There was a problem hiding this comment.
I intentionally did not include it; prototype objects no longer need to be indicated explicitly since we have the intrinsic dot syntax. The current ones are all legacy.
| 1. Let _errorsDesc_ be the PropertyDescriptor { [[Value]]: _errorsArray_, [[Writable]]: *true*, [[Enumerable]]: *false*, [[Configurable]]: *true* }. | ||
| 1. Perform ! DefinePropertyOrThrow ( _O_, `"errors"`, _errorsDesc_ ). | ||
| 1. Let O be ? OrdinaryCreateFromConstructor(_newTarget_, `"%AggregateError.prototype%"`, « [[ErrorData]] »). | ||
| 1. Let _errorsArray_ be ! CreateArrayFromList(? IterableToList(_errors_)). |
There was a problem hiding this comment.
I am not a fan of using multiple !/? macros in a single algorithm step. It's probably clear to a human, but it is not allowed by the current definition of the macros and it is not totally clear to me if it would be allowed after tc39/ecma262#1573.
(And there's no real reason not to just introduce a new variable and make it totally unambiguous.)
There was a problem hiding this comment.
We definitely do it in the spec in a number of places already; if 1573 wouldn’t allow it, we should ensure it will.
I could split it up, but I don’t think it adds clarity to do so.
There was a problem hiding this comment.
I tend to agree with:
(And there's no real reason not to just introduce a new variable and make it totally unambiguous.)
Happy to make this change separately though. No need to block this pull request.
|
This still LGTM. |
Partial fixes for #33.