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

Feature: AbortSignal support first value from last value from #6675

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Nov 15, 2021

feat(lastValueFrom): Adds support for cancellation with AbortSignal.

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 #6442

feat(firstValueFrom): now supports AbortSignal cancellation

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 #6442

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
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
@benlesh benlesh added 7.x Issues and PRs for version 6.x 8.x Issues and PRs for version 8.x AGENDA ITEM Flagged for discussion at core team meetings labels Nov 15, 2021
@benjamingr
Copy link
Contributor

Neat.

/**
* @deprecated Internal implementation detail. Do not construct error instances.
* Cannot be tagged as internal: https://github.com/ReactiveX/rxjs/issues/6269
*/
Copy link
Contributor

@josepot josepot Nov 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it should be tagged as "internal" because IMO this interface (and the AbortError class) shouldn't be publicly exported.

IMO if a user wants to check whether a Promise rejection was caused by an Abortion, the right way to check that would be: e instanceof Error && e.name === 'AbortError'.

What would happen, otherwise, if all libraries that want to support AbortSignal export their own version of AbortError? A user could accidentally import the AbortError instance from a different library and then trying to do e instanceof AbortError would return false for a rejection caused by an AbortSignal. That's why I think that in order to avoid that kind of ambiguity it's probably better if libraries that want to support AbortSignal do not expose their AbortError classes, and treat them as an internal implementation detail.

Although, I would be curios to know what @benjamingr thoughts are in this regard, because I'm sure that he must have already put some thought on what's the best way to detect rejections caused by an AbortSignal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josepot I wish I had a good answer but WHATWG are pretty much in "you shouldn't do that" camp and have recently made a change that means you can controller.abort(anyError) and then you are expected to throw signal.reason.

This is quite frustrating to me (possibly because I don't understand it well) but I don't have the capacity to engage with them and resolve it quite now.

Ref: whatwg/dom#1033

If this is important to RxJS - please also say it there.

Copy link
Contributor

@josepot josepot Nov 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjamingr thanks for your response! After I read all the comments in the issue that you shared, I ended up spending a lot of time trying to understand the history behind AbortController. Let's just say that I have a lot of thoughts and opinions about it 😅. However, in order to stay constructive, I will keep those thoughts to myself and I will try to propose a solution for RxJS that I think that should work for all users, despite the current (and hopefully temporary) misalignment between the DOM spec and the NodeJS API:

  • RxJS should keep its AbortError class as an internal implementation detail, since AFAIK doing e instanceof AbortError has never been the recommended way of identifying the AbortError exception.

  • When the onAbort event happens, then RxJS should first check whether signal.hasOwnProperty('reason'), and then it should reject using that reason if it exists, otherwise it should use a new instance of its internal AbortError as a fallback.

Rejecting abortions this way would guarantee that the RxJS implementation works for both kinds of ursers: DOM and NodeJS. Given the current state of things, I think that's the most sensible approach to stay compliant with the current misalignment.

Copy link

@ronag ronag Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference nodejs/node#41008

@benlesh
Copy link
Member Author

benlesh commented Jan 12, 2022

Core Team: Still uncertain about this direction, maybe we change the name of the config property to unstable_signal and let folks try it?

@benlesh
Copy link
Member Author

benlesh commented Mar 23, 2022

Core Team meeting:

  1. @benlesh says he's worried about diving into AbortSignal if it is never going to meet our ergonomics needs. But would still really, really like it if a native cancellation type like this worked out.
  2. Actionable: Reach out @benjamingr et al, about what needs we would really have to move the whole library this way.
  3. Alternative idea: have a signal_UNSAFE configuration to try out the waters.

@ronag
Copy link

ronag commented May 23, 2022

@benlesh @benjamingr Any hope of progress on this one?

@deadbeef84
Copy link

Anything we can do to move this forward?

@aikoven
Copy link

aikoven commented Nov 2, 2022

In the meantime you can use this third-party implementation: https://github.com/deeplay-io/abort-controller-x-rxjs

@benlesh benlesh removed the 7.x Issues and PRs for version 6.x label Dec 3, 2022
@benlesh
Copy link
Member Author

benlesh commented Jan 20, 2023

After using AbortSignal for a large project that was dealing with high-speed emissions, the performance of AbortSignal, in particular adding and removing event listeners for abort events, was so incredibly poor that I'm unwilling to introduce any flavor of it in RxJS.

We were waiting and waiting for this and thinking maybe we'd break ground on it in 8.x. And now I can't say that I think it's a good idea.

Use takeUntil(fromEvent(signal, 'abort')) in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.x Issues and PRs for version 8.x AGENDA ITEM Flagged for discussion at core team meetings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

firstValueFrom/lastValueFrom is missing AbortSignal support
6 participants