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

firstValueFrom/lastValueFrom is missing AbortSignal support #6442

Open
ronag opened this issue Jun 1, 2021 · 10 comments
Open

firstValueFrom/lastValueFrom is missing AbortSignal support #6442

ronag opened this issue Jun 1, 2021 · 10 comments

Comments

@ronag
Copy link

ronag commented Jun 1, 2021

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.
@antonkarsten
Copy link

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.

@ronag
Copy link
Author

ronag commented Oct 12, 2021

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.

@antonkarsten
Copy link

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.

@josepot
Copy link
Contributor

josepot commented Oct 12, 2021

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$));
};

@ronag
Copy link
Author

ronag commented Nov 15, 2021

@benjamingr

@benjamingr
Copy link
Contributor

@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
Copy link
Contributor

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

@benlesh
Copy link
Member

benlesh commented Nov 15, 2021

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

@benlesh
Copy link
Member

benlesh commented Nov 15, 2021

I'll have a go at this soon.

benlesh added a commit to benlesh/rxjs that referenced this issue Nov 15, 2021
Adds a feature to the `firstValueFrom` config to support passing an `AbortSignal` that can be used to unsubscribe from the underlying subscription. This will result in the returned promise rejecting with an `AbortError`, which is an error type belonging to the library at this point. This is because there is no consistent error type to throw across all supported runtimes that the user could check for.

related ReactiveX#6442
benlesh added a commit to benlesh/rxjs that referenced this issue Nov 15, 2021
Similar to the update to `firstValueFrom`. This adds a configuration option to unsubscribe from the underlying subscription with an `AbortSignal`. If aborted with a signal, the returned promise will reject with an `AbortError`.

resolves ReactiveX#6442
@deadbeef84
Copy link

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

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 a pull request may close this issue.

6 participants