-
Notifications
You must be signed in to change notification settings - Fork 12
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
Do we need the subscriber.closed
attribute?
#76
Comments
web-platform-tests/wpt#42219 (comment) has me just a little confused. It sounds like the AbortSignal passed into @benlesh am I right in thinking that |
I've cleaned up the code example in web-platform-tests/wpt#42219 (comment) that is trying to demonstrate the need for let firstTime = true;
const source = new Observable(subscriber => {
if (firstTime) {
firstTime = false;
source.subscribe({
complete: () => {
if (!subscriber.signal.aborted) {
subscriber.complete();
}
},
signal: subscriber.signal
})
} else {
subscriber.complete();
}
})
const ac = new AbortController();
source.subscribe({
complete: () => console.log('complete!'),
signal: ac.signal,
}); In Chromium with the Observables flag enabled, it seems to run fine / without issue. So I'm having a hard time figuring out why we need |
I think the point that the example is trying to make is something like: inside a subscriber's let innerSubscriber = null;
const observable = new Observable(subscriber => {
innerSubscriber = subscriber;
subscriber.complete();
});
const ac = new AbortController();
const signal = ac.signal;
observable.subscribe({
complete: () => {
console.log(signal.aborted);
// Because `signal.aborted` is observed as false, it might mislead
// people to thinking they can somehow call `subscriber.complete()` again!
// This would theoretically call this currently-running `complete()`
// handler recursively forever.
innerSubscriber.complete();
},
signal,
}); Is this accurate? |
OK the example above does indeed recursively run infinitely until the maximum stack size is reached. However, fixing this does not require an actual API change, so I'm still curious where the Conceptually "closing down" the subscription before actually invoking the |
Update: Ben and I are interested in introducing this as |
(If it's a getter or data property then it should be just |
This CL introduces an API, namely `subscriber.active` for determining the activity of an existing subscription. The `active` attribute gets set to false by the platform synchronously before the Observer's completion and error handlers are invoked. This has the effect of being observably `false` _within_ those handlers, unlike `subscriber.signal.aborted`. Please see the lengthy design notes in both WICG/observable#76 and WICG/observable#82. R=masonf@chromium.org For WPTs: Co-authored-by: ben@benlesh.com Bug: 1485981 Change-Id: I4604a80347ff30ff7be44a131bfdd3999b71252a
This CL introduces an API, namely `subscriber.active` for determining the activity of an existing subscription. The `active` attribute gets set to false by the platform synchronously before the Observer's completion and error handlers are invoked. This has the effect of being observably `false` _within_ those handlers, unlike `subscriber.signal.aborted`. Please see the lengthy design notes in both WICG/observable#76 and WICG/observable#82. R=masonf@chromium.org For WPTs: Co-authored-by: ben@benlesh.com Bug: 1485981 Change-Id: I4604a80347ff30ff7be44a131bfdd3999b71252a
Observables have a strict requirement that `complete()` and `error()` can only ever be called once, and specifically, once either one of these runs, no other `Observer` method can ever run again. The experimental Observable implementation in Blink previously did not respect this constraint, and this CL fixes this by essentially "closing" the subscription. ===Subsequent work=== Properly "closing" a subscription involves: 1. Ensuring `Subscriber#isActive` becomes false (WICG/observable#76 will track this). 2. Aborting the Subscriber's `AbortSignal`. This requires the AbortSignal that lives on `Subscriber` to differ from the one passed into `subscribe()`. See crrev.com/c/5017401. 3. Running the teardowns as the AbortSignal algorithms. Future CLs will implement the `addTeardown()` method in accordance with discussion in WICG/observable#22 (comment). R=masonf@chromium.org Bug: 1485981 Change-Id: I4ef9ab438e59326a49bd6561108b3bcc736845a5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5017293 Auto-Submit: Dominic Farolino <dom@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Commit-Queue: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1223921}
This CL introduces an API, namely `subscriber.active` for determining the activity of an existing subscription. The `active` attribute gets set to false by the platform synchronously before the Observer's completion and error handlers are invoked. This has the effect of being observably `false` _within_ those handlers, unlike `subscriber.signal.aborted`. Please see the lengthy design notes in both WICG/observable#76 and WICG/observable#82. R=masonf@chromium.org For WPTs: Co-authored-by: ben@benlesh.com Bug: 1485981 Change-Id: I4604a80347ff30ff7be44a131bfdd3999b71252a
This CL introduces an API, namely `subscriber.active` for determining the activity of an existing subscription. The `active` attribute gets set to false by the platform synchronously before the Observer's completion and error handlers are invoked. This has the effect of being observably `false` _within_ those handlers, unlike `subscriber.signal.aborted`. Please see the lengthy design notes in both WICG/observable#76 and WICG/observable#82. R=masonf@chromium.org For WPTs: Co-authored-by: ben@benlesh.com Bug: 1485981 Change-Id: I4604a80347ff30ff7be44a131bfdd3999b71252a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5017505 Commit-Queue: Dominic Farolino <dom@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Cr-Commit-Position: refs/heads/main@{#1224384}
In a review of some Observable constructor web platform tests, the
Subscriber#closed
attribute was brought up. I hadn't heard of this before, and while I can imagine this could be useful, a concrete practical use-case doesn't really jump out at me, so I'm filing this to spur some discussion about why this might be useful / if it is really needed.Please see all of the discussion starting at web-platform-tests/wpt#42219 (comment) and web-platform-tests/wpt#42219 (comment).
The text was updated successfully, but these errors were encountered: