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

Pass AbortSignal to callbacks passed to defer(), switchMap() etc #4051

Closed
wants to merge 2 commits into from

Conversation

felixfbecker
Copy link
Contributor

Description:
This is a small PoC for my proposal outlined in #3122 (comment)
Initially for defer() and switchMap().
AbortControllers only get instantiated if available in the environment so nothing breaks if they are not polyfilled.

Note that I am creating AbortControllers in the operator implementations - ideally those would be passed to the callback of the Observable constructor, but that was more effort to implement. My goal was just to create a PoC and keep the discussion going as this has been an ongoing pain point.

Related issue (if exists):
#3122 (comment)

@coveralls
Copy link

coveralls commented Aug 23, 2018

Coverage Status

Coverage decreased (-0.03%) to 96.918% when pulling bd15359 on felixfbecker:abortsignal into 9ea0ec9 on ReactiveX:master.

@benlesh
Copy link
Member

benlesh commented Aug 27, 2018

Looking at this, I'm not sure this is the proper approach right now. Considering right now AbortSignal isn't widely used or supported, I'd prefer just using Subscription in lieu of AbortSignal, and then figuring out a way to migrate Subscription to be an AbortController or an AbortController-shaped thing, given that Subscription is basically both an AbortController and an AbortSignal.

In the experimental branch, I'm pasing Subscription as a cancellation token of sorts to methods like forEach and toPromise. I hadn't thought about defer, etc.

Also, I'm keeping a close eye on this: tc39/proposal-cancellation#22

Which would resolve all of our problems, by allowing us to check a symbol and use what we get from that.

I really appreciate the work here, @felixfbecker, and we're definitely on the same page. I'm not ready to put this into the current stable release yet though, we'll definitely be trying some of this out in the next alpha.

@benlesh benlesh closed this Aug 27, 2018
@felixfbecker
Copy link
Contributor Author

Considering right now AbortSignal isn't widely used or supported

Are you referring to browser support? AbortSignal is supported by all modern browsers:

image

And it's already used by fetch and the WebAuthentication API (w3c/webauthn#544)

It's not supported in NodeJS of course, but I would take the same stance as with Promises - you can polyfill if you want to use it.

For the future, Node has shipped DOM APIs before - e.g. WHATWG URL

given that Subscription is basically both an AbortController and an AbortSignal.

I think this ascpect makes Subscription basically unusable for a general-purpose cancellation signal. If I pass a signal to a function, that function should have the ability to fork the token and cancel sub-operations, but it should never be able to cancel the operations of the caller.

In the experimental branch, I'm pasing Subscription as a cancellation token of sorts to methods like forEach and toPromise. I hadn't thought about defer, etc.

For me, defer, switchMap etc. are actually the more important use cases, because I see a lot that people want to do single-result Promise operations for every event in an Observable chain (e.g., call the fetch API or an API wrapper that returns a single result in reaction to events). Only rarely do you need to use a (multi-value) Observable from within an async function.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants