Open Jamesernator opened 2 months ago
Can you just do .takeUntil(stopWatcherController.signal.on('abort'))
instead?
Can you just do .takeUntil(stopWatcherController.signal.on('abort')) instead?
The signal can already be aborted, in which case the abort event will never fire again.
I guess AbortSignal could maybe override the on
method to provide an Observable that auto-completes in the case where the signal is already aborted. I'm not saying we have to go this route, just throwing ideas out.
I guess AbortSignal could maybe override the
on
method to provide an Observable that auto-completes in the case where the signal is already aborted. I'm not saying we have to go this route, just throwing ideas out.
This does feel like kind've a can of worms as it means .on
doesn't have one-to-one correspondence with events.
But also more generally there are plenty of events that could be argued should have the same behaviour, for example taskController.on("prioritychange")
could synchronously emit the initial priority, imageElement.on("load")
could resolve if the image is already loaded, etc.
If we did go this route, I think it would be better to have this ability across the board for supported events, e.g.:
signal.on("abort", { initial: true });
taskController.on("prioritychange", { initial: true });
// Also available for classic event listeners too
signal.addEventListener("abort", () => {}, { initial: true });
Also there is another general issue with AbortSignal
that is really beyond the scope of this issue, and that's that the "abort" event isn't even really a reliable mechanism of observing if the signal is aborted due to .dispatchEvent
and .stopImmediatePropagation()
.
Accepting AbortSignal
in .takeUntil
would allow .takeUntil
to sidestep the whole event stuff and use the internal hooks directly (like all other builtins do).
The signal can already be aborted, in which case the abort event will never fire again.
Yeah, this is one of the things that addTeardown
is working around. In RxJS we realized ages ago it was wildly unsafe and unergonomic to give people a cancellation/teardown mechanism that didn't execute teardowns immediately when someone tried to register one after it was already cancelled. This is because people forget to check things like signal.aborted
before they do signal.addEventListener('abort', fn, { once: true })
and cause themselves a memory leak.
new Observable((subscriber) => {
subscriber.complete(); // flags not active (or "closed"), and triggers abort
// Adding a teardown after things are already complete/errored/unsubscribed
// Will immediately execute that teardown
subscriber.addTeardown(() => console.log('fired immediately'));
});
If AbortSignal
had this behavior, addTeardown
wouldn't be necessary.
Related to #146
So I'm trying to figure out what the meat of this issue is. Is it just about AbortSignal
being insufficient for listening for signal abortion on, because it can be aborted before you start listening? I'm getting this from:
Also there is another general issue with AbortSignal that is really beyond the scope of this issue, and that's that the "abort" event isn't even really a reliable mechanism of observing if the signal is aborted
If that's what this issue is about, then I guess I'd point to https://github.com/WICG/observable/issues/146#issuecomment-2115583845 and we can discuss this elsewhere. But then reading:
Accepting AbortSignal in .takeUntil would allow .takeUntil to sidestep the whole event stuff and use the internal hooks directly (like all other builtins do).
I'm not sure I understand what to make of this. Specifically, I'm not sure what "like all other builtins do" means here, can you clarify @Jamesernator? Is the proposal here to allow takeUntil()
(and kin) to take in an AbortSignal
specifically to do the "is the passed-in signal already aborted" check internally so that the developer passing the signal in doesn't have to worry about it?
So I'm trying to figure out what the meat of this issue is. Is it just about AbortSignal being insufficient for listening for signal abortion on, because it can be aborted before you start listening?
The meat of the issue is that it would be convenient to pass abort signals to .takeUntil
(and as mentioned .on("abort")
is not reliable because the signal may be already aborted).
If that's what this issue is about, then I guess I'd point to https://github.com/WICG/observable/issues/146#issuecomment-2115583845 and we can discuss this elsewhere. But then reading:
That is merely a bonus advantage.
Is the proposal here to allow takeUntil() (and kin) to take in an AbortSignal specifically to do the "is the passed-in signal already aborted" check internally so that the developer passing the signal in doesn't have to worry about it?
Yes.
(and kin)
I don't know that there's any obvious use cases in the other methods, though I suppose if internally .takeUntil(arg)
performs Observable.from
on it's argument to get this behaviour then it'd affect .switchMap
and .flatMap
too.
Yeah I guess we'd need to figure out if just takeUntil()
could benefit from this, or if other methods should as well. Or even if we should muck around with from()
conversion logic. I think I'm going to label this issue as "possible future enhancement" for now, since while I think it could be useful, it probably could be a non-blocking enhancement that doesn't need immediate attention, and might be best taken care of at the AbortSignal
level anyways.
Hmm, I wonder if we should introduce a static method like Observable.complete()
, similar to the static Promise.resolve()
. Then you could do something like source.takeUntil(signal.aborted ? Observable.complete() : signa.on('abort'))
. It's not beautiful but it allows you to avoid having to remember which APIs treat AbortSignals in a special way and which don't.
I'm not sure we want to go off on our own and introduce the precedent (only in the Observable spec) that AbortSignals are sometimes treated specially, and can help trigger side effects if they are already aborted yet still passed in, without going through the DOM Standard / changes to AbortSignal itself.
But static APIs like Observable.complete()
help developers get around this pretty ergonomically, while still being very explicit.
It would be nice if addition to the existing overloads we had an overload for
.takeUntil
that accepted anAbortSignal
. An example of usage:The main difference compared to
.forEach(cb, { signal })
is that intermediate work wouldn't be interrupted (i.e. the cancellation happens between items, whereas if there were say a.flatMap
then that inner observable would be interrupted which isn't always desirable).