-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Feature: AbortSignal support first value from last value from #6675
Conversation
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
Neat. |
/** | ||
* @deprecated Internal implementation detail. Do not construct error instances. | ||
* Cannot be tagged as internal: https://github.com/ReactiveX/rxjs/issues/6269 | ||
*/ |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 doinge instanceof AbortError
has never been the recommended way of identifying theAbortError
exception. -
When the
onAbort
event happens, then RxJS should first check whethersignal.hasOwnProperty('reason')
, and then it should reject using that reason if it exists, otherwise it should use a new instance of its internalAbortError
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.
There was a problem hiding this comment.
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
Core Team: Still uncertain about this direction, maybe we change the name of the config property to |
Core Team meeting:
|
@benlesh @benjamingr Any hope of progress on this one? |
Anything we can do to move this forward? |
In the meantime you can use this third-party implementation: https://github.com/deeplay-io/abort-controller-x-rxjs |
After using 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 |
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 anAbortSignal
. If aborted with a signal, the returned promise will reject with anAbortError
.resolves #6442
feat(firstValueFrom): now supports AbortSignal cancellation
Adds a feature to the
firstValueFrom
config to support passing anAbortSignal
that can be used to unsubscribe from the underlying subscription. This will result in the returned promise rejecting with anAbortError
, 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