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

Spec from() static converter #160

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Spec from() static converter #160

wants to merge 18 commits into from

Conversation

keithamus
Copy link
Collaborator

@keithamus keithamus commented Jul 21, 2024

This fixes #159 by specifying the semantics of from(), while also then using that algorithm in takeUntil().


Preview | Diff

@domfarolino domfarolino changed the title add from() spec Spec from() static converter Jul 22, 2024
@domfarolino
Copy link
Collaborator

Thanks so much for adding this! It's been on my plate but alas I've not gotten to it yet. One thing is that we won't be able to land this without WPTs, so if you feel like writing those that would be great, otherwise I'm happy to do so, just FYI I am OOO for a bit coming up soon.

@petamoriken
Copy link

The ReadableStream.from spec is in the process of being changed to use the newly introduced async iterable WebIDL type, so Observable.from should be similarly specified.
whatwg/streams#1310

@keithamus keithamus force-pushed the add-from-spec branch 2 times, most recently from 3418ea1 to 99ed523 Compare August 12, 2024 14:02
@domfarolino
Copy link
Collaborator

I've started to revamp this a little bit to make sure the really subtle semantics of [Symbol.iterator] method access and how we deal with various values of that method's record match (a) the existing tests as well as (b) a bunch of new ones I'm adding in https://chromium-review.googlesource.com/c/chromium/src/+/5824955.

Copy link
Collaborator

@benlesh benlesh left a comment

Choose a reason for hiding this comment

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

Most of this looks okay, just a couple of comments.

We're also missing ArrayLike. Which is supported in Array.from() et al.

ArrayLike should handle any object with a length and numeric keys/indices.

It's probably not a big deal, and we can add it later, I suppose, but I think that Iterator.from() handles it? (I'll have to double check)... Array.from() definitely does.

1. <i id=from-observable-conversion><b>From Observable</b></i>: If |value|'s [=specific type=]
is an {{Observable}}, then return |value|.

1. <i id=from-async-iterable-conversion><b>From async iterator</b></i>: Let
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's important in here to make sure that we add a check and call to the async iterator's return() method, if it has one. Because if it's a generator, I'd expect gen.return() to be called so it hits the finally block:

function sleep(ms) {
  return new Promise(resolve => setTimeout(resolve, ms));
}

async function* infiniteGenerator() {
  let n = 0;
  try {
    while (true) {
      await sleep(100);
      yield n++;
    }
  } finally {
    console.log('this must be called!');
  }
}

const ac = new AbortController();

Observable.from(infiniteGenerator())
  .subscribe(console.log, { signal: ac.signal });
  
setTimeout(() => {
  ac.abort(); // should cause log of "this must be called!"
}, 333);

1. If |iteratorMethodRecord|'s \[[Value]] is undefined or null, then jump to the step labeled <a
href=#from-promise-conversion>From Promise</a>.

1. If [$IsCallable$](|iteratorMethodRecord|'s \[[Value]]) is false, then [=exception/throw=] a
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to what I said above about async generators. This has to accommodate generators.

function* infiniteGenerator() {
  let n = 0;
  try {
    while (true) {
      yield n++;
    }
  } finally {
    console.log('this must be logged!');
  }
}

const ac = new AbortController();

Observable.from(infiniteGenerator())
  .subscribe((n) => {
    console.log(n);
    if (n === 4) ac.abort(); // Should force the logging of "this must be logged!"
  }, { signal: ac.signal })

@domfarolino
Copy link
Collaborator

@benlesh Can you make a WPT PR with tests for the cases you're describing please?

@benlesh
Copy link
Collaborator

benlesh commented Aug 29, 2024

@domfarolino :) It was in-process while you were typing that: web-platform-tests/wpt#47874

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 30, 2024
See WICG/observable#160, which specs the
`Observable#from()` semantics, matching these tests. See also
https://crbug.com/363015168 which describes the ways in which our
current implementation of `Observable#from()`'s detection semantics
are overbroad.

This CL makes the implementation of the "detection semantics" match the
desired behavior outlined in that issue, and adds a bunch of tests.

R=masonf@chromium.org

Bug: 363015168, 40282760
Change-Id: Id6cfdd45c44286b298e107635e4283b018f50aaf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5824955
Commit-Queue: Dominic Farolino <dom@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1349019}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 30, 2024
See WICG/observable#160, which specs the
`Observable#from()` semantics, matching these tests. See also
https://crbug.com/363015168 which describes the ways in which our
current implementation of `Observable#from()`'s detection semantics
are overbroad.

This CL makes the implementation of the "detection semantics" match the
desired behavior outlined in that issue, and adds a bunch of tests.

R=masonf@chromium.org

Bug: 363015168, 40282760
Change-Id: Id6cfdd45c44286b298e107635e4283b018f50aaf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5824955
Commit-Queue: Dominic Farolino <dom@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1349019}
Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

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

Quick review just to document the current state of things. I think we're just about ready modulo the async iterator stuff, which I think needs a little work I've outlined below.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 31, 2024
…rator] semantics (1/2), a=testonly

Automatic update from web-platform-tests
DOM: Fix `Observable#from()` [Symbol.iterator] semantics (1/2)

See WICG/observable#160, which specs the
`Observable#from()` semantics, matching these tests. See also
https://crbug.com/363015168 which describes the ways in which our
current implementation of `Observable#from()`'s detection semantics
are overbroad.

This CL makes the implementation of the "detection semantics" match the
desired behavior outlined in that issue, and adds a bunch of tests.

R=masonf@chromium.org

Bug: 363015168, 40282760
Change-Id: Id6cfdd45c44286b298e107635e4283b018f50aaf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5824955
Commit-Queue: Dominic Farolino <dom@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1349019}

--

wpt-commits: e74098ed10aa0a4fc6ad6b8d6e354895addd00d1
wpt-pr: 47881
Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

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

A few more things I've noted. I'll work on these and try and fix most of them today.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Sep 3, 2024
…rator] semantics (1/2), a=testonly

Automatic update from web-platform-tests
DOM: Fix `Observable#from()` [Symbol.iterator] semantics (1/2)

See WICG/observable#160, which specs the
`Observable#from()` semantics, matching these tests. See also
https://crbug.com/363015168 which describes the ways in which our
current implementation of `Observable#from()`'s detection semantics
are overbroad.

This CL makes the implementation of the "detection semantics" match the
desired behavior outlined in that issue, and adds a bunch of tests.

R=masonf@chromium.org

Bug: 363015168, 40282760
Change-Id: Id6cfdd45c44286b298e107635e4283b018f50aaf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5824955
Commit-Queue: Dominic Farolino <dom@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1349019}

--

wpt-commits: e74098ed10aa0a4fc6ad6b8d6e354895addd00d1
wpt-pr: 47881
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Sep 6, 2024
…rator] semantics (1/2), a=testonly

Automatic update from web-platform-tests
DOM: Fix `Observable#from()` [Symbol.iterator] semantics (1/2)

See WICG/observable#160, which specs the
`Observable#from()` semantics, matching these tests. See also
https://crbug.com/363015168 which describes the ways in which our
current implementation of `Observable#from()`'s detection semantics
are overbroad.

This CL makes the implementation of the "detection semantics" match the
desired behavior outlined in that issue, and adds a bunch of tests.

R=masonfchromium.org

Bug: 363015168, 40282760
Change-Id: Id6cfdd45c44286b298e107635e4283b018f50aaf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5824955
Commit-Queue: Dominic Farolino <domchromium.org>
Reviewed-by: Mason Freed <masonfchromium.org>
Cr-Commit-Position: refs/heads/main{#1349019}

--

wpt-commits: e74098ed10aa0a4fc6ad6b8d6e354895addd00d1
wpt-pr: 47881

UltraBlame original commit: ef2aac8ce3d8e023f93a55c7593551be16b64d72
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Sep 6, 2024
…rator] semantics (1/2), a=testonly

Automatic update from web-platform-tests
DOM: Fix `Observable#from()` [Symbol.iterator] semantics (1/2)

See WICG/observable#160, which specs the
`Observable#from()` semantics, matching these tests. See also
https://crbug.com/363015168 which describes the ways in which our
current implementation of `Observable#from()`'s detection semantics
are overbroad.

This CL makes the implementation of the "detection semantics" match the
desired behavior outlined in that issue, and adds a bunch of tests.

R=masonfchromium.org

Bug: 363015168, 40282760
Change-Id: Id6cfdd45c44286b298e107635e4283b018f50aaf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5824955
Commit-Queue: Dominic Farolino <domchromium.org>
Reviewed-by: Mason Freed <masonfchromium.org>
Cr-Commit-Position: refs/heads/main{#1349019}

--

wpt-commits: e74098ed10aa0a4fc6ad6b8d6e354895addd00d1
wpt-pr: 47881

UltraBlame original commit: ef2aac8ce3d8e023f93a55c7593551be16b64d72
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Sep 9, 2024
…rator] semantics (1/2), a=testonly

Automatic update from web-platform-tests
DOM: Fix `Observable#from()` [Symbol.iterator] semantics (1/2)

See WICG/observable#160, which specs the
`Observable#from()` semantics, matching these tests. See also
https://crbug.com/363015168 which describes the ways in which our
current implementation of `Observable#from()`'s detection semantics
are overbroad.

This CL makes the implementation of the "detection semantics" match the
desired behavior outlined in that issue, and adds a bunch of tests.

R=masonf@chromium.org

Bug: 363015168, 40282760
Change-Id: Id6cfdd45c44286b298e107635e4283b018f50aaf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5824955
Commit-Queue: Dominic Farolino <dom@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1349019}

--

wpt-commits: e74098ed10aa0a4fc6ad6b8d6e354895addd00d1
wpt-pr: 47881
aarongable pushed a commit to chromium/chromium that referenced this pull request Sep 17, 2024
This CL implements async iterator support for Observables, and adds tons
of WPTs exercising subtle functionality of iterable and async iterable
Observable conversion semantics. It implements the spec text in
WICG/observable#160, and is a follow-up to
https://crrev.com/c/5840672, which brings async iterable support to core
bindings code.

This CL amounts to a partial implementation of async iterable support;
what's missing and what will be included as a follow-up is:

 1. Support for calling the Async Iterator's `return()` method [1] when
    an Observable — when consuming an async iterable — aborts its
    subscription before iterable exhaustion.
 3. A possible refactor to move some of the logic that handles the
    `ScriptIterator` into `ScriptIterator` itself, per discussion in
    [2].

[1]: https://tc39.es/ecma262/#sec-asynciterator-interface
[2]: https://chromium-review.googlesource.com/c/chromium/src/+/5840672/comment/72df95a9_dd32d801/

R=masonf@chromium.org

Bug: 40282760, 363015168
Change-Id: I5f31f4028613245025de71b8975fc92e9d1def0a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5850509
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1356228}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 17, 2024
This CL implements async iterator support for Observables, and adds tons
of WPTs exercising subtle functionality of iterable and async iterable
Observable conversion semantics. It implements the spec text in
WICG/observable#160, and is a follow-up to
https://crrev.com/c/5840672, which brings async iterable support to core
bindings code.

This CL amounts to a partial implementation of async iterable support;
what's missing and what will be included as a follow-up is:

 1. Support for calling the Async Iterator's `return()` method [1] when
    an Observable — when consuming an async iterable — aborts its
    subscription before iterable exhaustion.
 3. A possible refactor to move some of the logic that handles the
    `ScriptIterator` into `ScriptIterator` itself, per discussion in
    [2].

[1]: https://tc39.es/ecma262/#sec-asynciterator-interface
[2]: https://chromium-review.googlesource.com/c/chromium/src/+/5840672/comment/72df95a9_dd32d801/

R=masonf@chromium.org

Bug: 40282760, 363015168
Change-Id: I5f31f4028613245025de71b8975fc92e9d1def0a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5850509
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1356228}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 17, 2024
This CL implements async iterator support for Observables, and adds tons
of WPTs exercising subtle functionality of iterable and async iterable
Observable conversion semantics. It implements the spec text in
WICG/observable#160, and is a follow-up to
https://crrev.com/c/5840672, which brings async iterable support to core
bindings code.

This CL amounts to a partial implementation of async iterable support;
what's missing and what will be included as a follow-up is:

 1. Support for calling the Async Iterator's `return()` method [1] when
    an Observable — when consuming an async iterable — aborts its
    subscription before iterable exhaustion.
 3. A possible refactor to move some of the logic that handles the
    `ScriptIterator` into `ScriptIterator` itself, per discussion in
    [2].

[1]: https://tc39.es/ecma262/#sec-asynciterator-interface
[2]: https://chromium-review.googlesource.com/c/chromium/src/+/5840672/comment/72df95a9_dd32d801/

R=masonf@chromium.org

Bug: 40282760, 363015168
Change-Id: I5f31f4028613245025de71b8975fc92e9d1def0a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5850509
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1356228}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 20, 2024
The IteratorRecord#return() function exists as an optional method that
sync and async iterator records can supply [1] [2]. They allow for the
language, or any consumer of an iterable, to signal to the iterable that
the consumer will stop consuming values prematurely (i.e., before
exhaustion).

This method must be invoked when the consumer aborts its subscription
to an Observable that was derived from an iterable. The abort reason is
supplied to the `return()` iterator function for completeness. This CL:
  1. Adds tests for sync & async iterables
  2. Implements this for async iterables

A follow-up CL will implement this for sync iterables.

The semantics are specified in
WICG/observable#160.

[1]:
https://tc39.es/ecma262/#table-iterator-interface-optional-properties
[2]: https://tc39.es/ecma262/#table-async-iterator-optional

Bug: 40282760
Change-Id: Ie1091b24b233afecdec572feadc129bcc8a2d4d3
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 20, 2024
The IteratorRecord#return() function exists as an optional method that
sync and async iterator records can supply [1] [2]. They allow for the
language, or any consumer of an iterable, to signal to the iterable that
the consumer will stop consuming values prematurely (i.e., before
exhaustion).

This method must be invoked when the consumer aborts its subscription
to an Observable that was derived from an iterable. The abort reason is
supplied to the `return()` iterator function for completeness. This CL:
  1. Adds tests for sync & async iterables
  2. Implements this for async iterables

A follow-up CL will implement this for sync iterables.

The semantics are specified in
WICG/observable#160.

[1]:
https://tc39.es/ecma262/#table-iterator-interface-optional-properties
[2]: https://tc39.es/ecma262/#table-async-iterator-optional

Bug: 40282760
Change-Id: Ie1091b24b233afecdec572feadc129bcc8a2d4d3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

takeUntil unclear on how to coerce type
4 participants