Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

[WIP] Add first spec draft #15

Merged
merged 8 commits into from
Mar 25, 2019
Merged

Conversation

chicoxyzzy
Copy link
Member

@chicoxyzzy chicoxyzzy commented Mar 1, 2019

AggregateError is not implemented in this version yet. I'll add it when we'll reach an agreement on
#14 and current text.

update: AggregateError added

@chicoxyzzy chicoxyzzy requested a review from mathiasbynens March 1, 2019 11:35
Copy link
Member

@mathiasbynens mathiasbynens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Please add yourself to the contributors: at the top.

@mathiasbynens
Copy link
Member

This looks great! I'll take a closer look soon.

spec.html Outdated Show resolved Hide resolved
<h1>Runtime Semantics: PerformPromiseAny ( _iteratorRecord_, _constructor_, _resultCapability_ )</h1>
<p>When the PerformPromiseAny abstract operation is called with arguments _iteratorRecord_, _constructor_, and _resultCapability_, the following steps are taken:</p>
<emu-alg>
1. Assert: ! IsConstructor(_constructor_) is *true*.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an assertion about iteratorRecord?

1. Assert: ! IsConstructor(_constructor_) is *true*.
1. Assert: _resultCapability_ is a PromiseCapability Record.
1. Let _values_ be a new empty List.
1. Let _remainingElementsCount_ be a new Record { [[Value]]: 1 }.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems weird to have a record here and pass it around. Is keeping a count like this the only solution?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at existing ECMAScript Specification Types, Record is most suitable type here. It's used in other Promise namespace functions as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh sure, i wasn't suggesting a JS object or anything :-) more that, is passing around a counter the only viable approach?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I got it. Not sure to be honest 🤔 I need to do some more research to answer this question 🕵️‍♀️

@chicoxyzzy chicoxyzzy requested review from mathiasbynens and ljharb and removed request for mathiasbynens March 1, 2019 19:20
@chicoxyzzy
Copy link
Member Author

chicoxyzzy commented Mar 1, 2019

Sorry for re-requesting review. Seems like it's a new GitHub feature. I thought it's kinda refresh button like when someone adds new commit to PR while it's already open in a tab 😅
image

<p>When the *AggregateError* function is called with arguments _errors_ and _message_, the following steps are taken:</p>
<emu-alg>
1. If NewTarget is *undefined*, let _newTarget_ be the active function object, else let _newTarget_ be NewTarget.
1. Let O be ? OrdinaryCreateFromConstructor(_newTarget_, `"%AggregateErrorPrototype%"`, « [[ErrorData]] »).
Copy link
Member Author

@chicoxyzzy chicoxyzzy Mar 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb should this be

Suggested change
1. Let O be ? OrdinaryCreateFromConstructor(_newTarget_, `"%AggregateErrorPrototype%"`, « [[ErrorData]] »).
1. Let O be ? OrdinaryCreateFromConstructor(_newTarget_, `"%AggregateError.prototype%"`, « [[ErrorData]] »).

instead?
ref: https://github.com/tc39/ecma262/pull/1376/files

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good one

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chicoxyzzy once that PR lands, yes, it decidedly should be :-) until then, tho, you'd need to create a new intrinsic for the prototype for this spec to be correct.

1. Set _values_[_index_] to _x_.
1. Set _remainingElementsCount_.[[Value]] to _remainingElementsCount_.[[Value]] - 1.
1. If _remainingElementsCount_.[[Value]] is 0, then
1. Let _valuesArray_ be CreateArrayFromList(_values_).
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should be something like

        Let _valuesArray_ be AggregateError(_values_, ???).

I'm not sure

Copy link
Collaborator

@bakkot bakkot Mar 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't yet have a convention for how to specify an error object which has a required property. I think I would do something like

1. Let _errorsArray_ be CreateArrayFromList(_values_).
1. Let _exception_ be a newly created AggregateError object.
1. Perform ! CreateDataProperty(_exception_, `"errors"`, _errorsArray_).
1. Return ? Call(*undefined*, _promiseCapability_.[[Reject]], &laquo; _exception_ &raquo;).

The "a newly created WhateverError object" phrasing is used elsewhere. Doing it this way hopefully makes it clear that engines are required to provide .errors but are free to make message whatever they feel is appropriate, just as with other errors thrown by builtins.

@chicoxyzzy
Copy link
Member Author

chicoxyzzy commented Mar 6, 2019

I just added spec text for AggregateError proposed by @bakkot in #14 (comment)

@mathiasbynens
Copy link
Member

I’ll merge this as-is and we can take it from there. Thanks!

@mathiasbynens mathiasbynens merged commit 99ba5f0 into tc39:master Mar 25, 2019
@chicoxyzzy chicoxyzzy deleted the spec_draft branch March 25, 2019 19:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants