takeUntil - accept abort signals

[Jamesernator]

It would be nice if addition to the existing overloads we had an overload for .takeUntil that accepted an AbortSignal. An example of usage:

import chokidar from "chokidar";

const stopWatcherController = new AbortController();

async function onChangeTask(kind, path) {
    // ...
}

await chokidar.watch("**/*")
    // Assuming Node EventEmitter gets some method to convert to observables
    .toObservable("all")
    .takeUntil(stopWatcherController.signal)
    .forEach(onChangeTask);
    
console.log("Watcher closed successfully");

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).

[domfarolino]

Can you just do .takeUntil(stopWatcherController.signal.on('abort')) instead?

[Jamesernator]

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.

[domfarolino]

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.

[Jamesernator]

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).

[benlesh]

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

[domfarolino]

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 #146 (comment) 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?

[Jamesernator]

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 #146 (comment) 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.

[domfarolino]

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.

[domfarolino]

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.