rxjs icon indicating copy to clipboard operation
rxjs copied to clipboard

firstValueFrom/lastValueFrom is missing AbortSignal support

Open ronag opened this issue 3 years ago • 10 comments

Promise based API's should generally also accept a signal property which is passed an AbortSignal instance.

const ac = new AbortController()
const promise = await rxjs.firstValueFrom(x$, { signal: ac.signal })
setTimeout(() => ac.abort(), 1e3)
await promise // Throws abort error.

ronag avatar Jun 01 '21 10:06 ronag

Hi @ronag I see your point, but maybe the idea is to use use something like

firstValueFrom(x$.pipe(takeUntil(timer(1e3))));

if you want to cancel. this will than reject your promise.

antonkarsten avatar Oct 12 '21 13:10 antonkarsten

No... the correct way would be something like:

await rxjs.firstValueFrom(x$.pipe(rxjs.fromEvent(signal, 'abort'), rxjs.throwError(() => new AbortError()))

Which is kind of a mouthfull... in general promise APi's should support the signal parameter.

ronag avatar Oct 12 '21 13:10 ronag

Why do you think that is the correct way? Why do you think promise API's should support the AbortSignal?

Dont get me wrong. I do not know what is intended. Im just trying to understand the issue and help if I can.

The way I see it is that the AbortSignal is often used as a mechanism to cancel (reject) promises. But the firstValueFrom already has an rxjs-way to cancel it. So it might be intentional that it does not accept an AbortSignal.

antonkarsten avatar Oct 12 '21 13:10 antonkarsten

Taking into account that AbortSignal is not standard JS and that the TC39 proposal for ECMAScript cancellation is still at its very early stages, I don't see why RxJS should support AbortSignal Promises.

Also, the implementation of this custom operator seems to be pretty trivial... I haven't tested it, but I think that this should do it:

const abortableFirstValueFrom = <T>(
  source$: Observable<T>,
  signal: AbortSignal
): Promise<T> => {
  if (signal.aborted) return Promise.reject(new AbortError())

  const signalError$ = fromEvent(signal, "abort").pipe(
    map(() => {
      throw new AbortError();
    })
  );

  return firstValueFrom(merge(source$, signalError$));
};

josepot avatar Oct 12 '21 14:10 josepot

@benjamingr

ronag avatar Nov 15 '21 08:11 ronag

@benlesh is someone I trust in both RxJS and AbortSignal and has provided API feedback before.

In my opinion @ronag is in the right here and the API should take a signal.

benjamingr avatar Nov 15 '21 15:11 benjamingr

Why do you think that is the correct way? Why do you think promise API's should support the AbortSignal?

That's a good question.

This is related to discussions (e.g. https://github.com/ReactiveX/rxjs/issues/5545 ) in RxJS about building on and supporting modern cross-platform primitives.

It is the "official guidance" of Node.js and WHATWG that the way to handle cancellation in promise returning APIs is with AbortSignal. We have also added AbortSignal support to all of our own APIs in recent versions.

We don't own the way people write code. RxJS is welcome to do something different from Node.js here - and we are not remotely forcing (or even strongly suggesting) adoption.

I think @ronag's ask is reasonable (support AbortSignal) in APIs that return promises (like .forEach and firstValueFrom).

benjamingr avatar Nov 15 '21 15:11 benjamingr

I agree. I would also like to add it to forEach, but that needs to be more carefully considered, I think.

benlesh avatar Nov 15 '21 17:11 benlesh

I'll have a go at this soon.

benlesh avatar Nov 15 '21 17:11 benlesh

@benlesh I dont see how AbortSignal having bad performance in some environments should block supporting signals in rxjs. This would be similar to not supporting promises because callbacks are faster, no? If performance is that big of an issue, mention it in the docs so users can opt for other alternatives where performance is an issue.

deadbeef84 avatar Oct 19 '23 09:10 deadbeef84