Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Normative: Promise.{all,allSettled,race} should check resolve is a function before opening their iterable #1912

Merged
merged 1 commit into from
Jun 10, 2020
Merged

Normative: Promise.{all,allSettled,race} should check resolve is a function before opening their iterable #1912

merged 1 commit into from
Jun 10, 2020

Conversation

kmiller68
Copy link
Contributor

@kmiller68 kmiller68 commented Mar 22, 2020 •
edited by ljharb
Loading

This change makes promise.{all,allSettled,race} behave more like a canonical JS implementation. In particular, the resolve property is now checked to be callable before beginning iteration on the first argument.

Closes #1902.

@kmiller68 kmiller68 changed the title This change makes promise.{all,allSettled,race} behave more like a Promise.{all,allSettled,race} should check resolve is a function before opening their iteratable Mar 22, 2020
@bakkot bakkot added needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Mar 22, 2020
bakkot
bakkot reviewed Mar 23, 2020
View reviewed changes
spec.html Outdated Show resolved Hide resolved
ljharb
ljharb reviewed Mar 23, 2020
View reviewed changes
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
@ljharb ljharb requested review from michaelficarra, syg and a team March 23, 2020 18:28
ljharb pushed a commit to tc39/agendas that referenced this pull request Apr 1, 2020
@kmiller68 @ljharb
2020.06: add Promise needs-consensus PR
3d6ea07 
ref tc39/ecma262#1912
@shvaikalesh shvaikalesh mentioned this pull request Apr 28, 2020
Merged
@ljharb ljharb added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Jun 1, 2020
This was referenced Jun 3, 2020
Merged
Merged
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 3, 2020
@anba
Bug 1642683: Retrieve 'resolve' property before the iterable in the P…
dd55e1a 
…romise combinators. r=arai

Apply the updates from <tc39/ecma262#1912>.

Differential Revision: https://phabricator.services.mozilla.com/D77838
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Jun 4, 2020
@anba
Bug 1642683: Retrieve 'resolve' property before the iterable in the P…
9f8d53a 
…romise combinators. r=arai

Apply the updates from <tc39/ecma262#1912>.

Differential Revision: https://phabricator.services.mozilla.com/D77838
mathiasbynens pushed a commit to tc39/proposal-promise-any that referenced this pull request Jun 4, 2020
@shvaikalesh
Retrieve Promise.resolve before GetIterator call (#65)
24998a0 
Per TC39 consensus on tc39/ecma262#1912.
@kmiller68 kmiller68 force-pushed the promise-functions-iteration-consistency branch from 54602e3 to abc3c32 Compare June 4, 2020 23:10
@kmiller68
Copy link
Contributor Author

Looks, like @shvaikalesh graciously updated test262 for me with tc39/test262#2648. (Thanks!)

I rebased against the current spec and fixed my summaries to match the newer style. I noticed that we use the phrasing The abstract operation <Op> takes argument <Arg>., which seems odd to me but I kept it for consistency.

ljharb
ljharb reviewed Jun 5, 2020
View reviewed changes
spec.html Outdated Show resolved Hide resolved
@kmiller68 kmiller68 added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Jun 6, 2020
syg
syg approved these changes Jun 8, 2020
View reviewed changes
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

lgtm

@ljharb ljharb requested review from bakkot and a team June 8, 2020 23:54
@ljharb ljharb self-assigned this Jun 8, 2020
backwardn pushed a commit to backwardn/v8 that referenced this pull request Jun 9, 2020
@marjakh
[Promise combinators] Spec change: check "resolve" early
539979f 
Promise.{all,allSettled,any,race} should check resolve is a function before
opening their iteratable.

PR: tc39/ecma262#1912

PR for Promise.any: tc39/proposal-promise-any#65

This CL includes the following cleanup changes:
- Made it more explicit that the constructor is a Constructor.
- Removed unnecessary nested try blocks (a try can have both a catch and a label).
- Moved commonly used definitions out of promise-race.tq where they don't belong.
- Made the parameter order of PerformPromiseAll match the spec.


Bug: v8:10578
Change-Id: I9deb5d5106db7350a0d0ad52f165ff2469e7074b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2232544
Commit-Queue: Marja Hölttä <[email protected]>
Reviewed-by: Igor Sheludko <[email protected]>
Reviewed-by: Shu-yu Guo <[email protected]>
Cr-Commit-Position: refs/heads/master@{#68260}
bakkot
bakkot reviewed Jun 10, 2020
View reviewed changes
spec.html Show resolved Hide resolved
@ljharb ljharb changed the title Promise.{all,allSettled,race} should check resolve is a function before opening their iteratable Normative: Promise.{all,allSettled,race} should check resolve is a function before opening their iterable Jun 10, 2020
ljharb pushed a commit to kmiller68/ecma262 that referenced this pull request Jun 10, 2020
@kmiller68 @ljharb
Normative: Promise.{all,allSettled,race} should check resolve is a fu…
6ca6f94 
…nction before opening their iterable (tc39#1912)

This change makes promise.{all,allSettled,race} behave more like a
canonical JS implementation.  In particular, the resolve property is
now checked to be callable before beginning iteration on the first
argument.

Closes tc39#1902.
@ljharb ljharb force-pushed the promise-functions-iteration-consistency branch from dc589fa to 6ca6f94 Compare June 10, 2020 05:21
@kmiller68 @ljharb
Normative: Promise.{all,allSettled,race} should check resolve is a fu…
855093b 
…nction before opening their iterable (tc39#1912)

This change makes promise.{all,allSettled,race} behave more like a
canonical JS implementation.  In particular, the resolve property is
now checked to be callable before beginning iteration on the first
argument.

Closes tc39#1902.
@ljharb ljharb force-pushed the promise-functions-iteration-consistency branch from 6ca6f94 to 855093b Compare June 10, 2020 05:22
@ljharb ljharb merged commit 855093b into tc39:master Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bakkot bakkot bakkot approved these changes

@syg syg syg approved these changes

@ljharb ljharb ljharb approved these changes

@michaelficarra michaelficarra Awaiting requested review from michaelficarra

Assignees

@ljharb ljharb

Labels
has consensus This has committee consensus. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Promise.{all,allSettled,race} should check resolve is a function before opening their iteratable
4 participants
@kmiller68 @ljharb @syg @bakkot