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.

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

Pre-Stage-3 reviews #14

Open
2 of 5 tasks
js-choi opened this issue Dec 14, 2021 · 14 comments
Open
2 of 5 tasks

Pre-Stage-3 reviews #14

js-choi opened this issue Dec 14, 2021 · 14 comments
Labels
question Further information is requested

Comments

@js-choi
Copy link
Collaborator

js-choi commented Dec 14, 2021

As per the TC39 process document.

Editors:

@js-choi js-choi added the question Further information is requested label Dec 15, 2021
@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jan 3, 2022

I opened #19 with an observation/idea, but apart from that the spec looks good. ✔️

There is a minor editorial bug: the Abstract Closure should capture thisArg.

@js-choi
Copy link
Collaborator Author

js-choi commented Jan 5, 2022

Since #20 has been merged, I have marked your review as completed, @nicolo-ribaudo. Thank you again.

@ljharb
Copy link
Member

ljharb commented Jan 24, 2022

For my review:

  • I'd love to see the %Array.prototype.values% simplification upstreamed to Array.from in 262 as a PR, both as an improvement and also to minimize from/fromAsync diffs
  • after the spec PR lands, it'd be great to figure out if there's a way to use one or more common AOs to maximally share steps between from and fromAsync

Otherwise, LGTM.

@js-choi
Copy link
Collaborator Author

js-choi commented Jul 12, 2022

@nicolo-ribaudo, @ljharb:
The spec has had significant changes due to the reversion of #20 in #27, so that its behavior would better match for await and AsyncIterator.prototype.toArray (see
#19 (comment)). Hopefully we’ve got everything set now.

I’d like to request another review of the proposal. I would like to try advancing this proposal to Stage 3 at the 2022-09 plenary meeting. Thank you again; please let me know here if you may not be able to adequately review by 2022-09-01. Thank you sincerely for your time.

@js-choi
Copy link
Collaborator Author

js-choi commented Sep 8, 2022

@ljharb, @nicolo-ribaudo: This is another ping before next week’s plenary meeting to re-review the proposal spec within the next few days. Let us know once you re-review—or if you anticipate that you won’t be able to properly review it by then. Thank you!

@nicolo-ribaudo
Copy link
Member

I will do this on Friday, sorry for being late.

@ljharb
Copy link
Member

ljharb commented Sep 8, 2022

Spec seems good to me.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Sep 14, 2022

I will do this on Friday, sorry for being late.

I forgot to reply before plenary, but I re-reviewed the proposal spec and it still looks good.

(Also, I like how you extended AsyncBlockStart because I need the same for another proposal!)

@js-choi
Copy link
Collaborator Author

js-choi commented Sep 14, 2022

We got Stage 3, conditional on editor approval.

@syg, @michaelficarra, @bakkot: Please let us know when you have been able to review the spec.

(The typo that @waldemarhorwat found during the plenary has already been fixed.)

@mgaudet
Copy link

mgaudet commented Mar 29, 2023

This is already shipping in Safari Technical Preview, despite being only 'conditionally' stage 3. Firefox has an implementation and is ready to start shipping to nightly when this is officially stage 3.

Any updates on editorial review @syg, @bakkot or @michaelficarra ?

@michaelficarra
Copy link
Member

michaelficarra commented Mar 30, 2023

@mgaudet It'd be easiest for me to do a proper review if the proposal was opened as a PR against ecma262.

We should land tc39/ecma262#2942 and tc39/ecma262#3021 first, and we should deduplicate the async-to-sync fallback behaviour that's present in both GetIterator and fromAsync.

It looks like we construct the this value more than once when the parameter is an array-like. This seems like a mistake. I think steps 3.e/3.f are meant to be nested under step 3.j. This is a normative change and committee should be notified. edit: Looks like you already noticed this as well in #35.

Step 3.k.viii seems unnecessary.

In 3.k.vii.4.b, we should be using set, not let. Ecmarkup linting should've caught this. Maybe you need to upgrade or enable the strict linting flag? If you open a PR, the CI will also run these checks. edit: #40

@mgaudet
Copy link

mgaudet commented Mar 30, 2023

I'm not the author of this proposal -- just trying to get this into a state where Firefox can ship our implementation.

@js-choi
Copy link
Collaborator Author

js-choi commented May 11, 2023

My apologies for the radio silence; I’ve been swamped by work since last winter. I’ve been talking with @michaelficarra privately about next steps:

Thanks everyone for your patience.

@js-choi
Copy link
Collaborator Author

js-choi commented Nov 1, 2023

#40 has been merged. #35 has also been merged, as per the plenary consensus on 2023-05.

As far as I know, our next steps for actual Stage 3 are:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants