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.

Sign up for GitHub

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

Jump to bottom

Feature: AbortSignal support #5591

Closed
wants to merge 5 commits into from
Closed

Feature: AbortSignal support #5591

wants to merge 5 commits into from

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Jul 15, 2020 •
edited
Loading

This is a flag in the ground for adding support for AbortSignal to RxJS.

This is a non-breaking change that adds support for passing AbortSignal to both subscribe and forEach calls of Observable. See commits.

Related #5545 and #3122

@benlesh benlesh requested a review from cartant July 15, 2020 17:02
@benlesh
Copy link
Member Author

benlesh commented Jul 15, 2020

NOTE: This does not yet add passing of an AbortSignal as an argument to the initialization function of Observable. That would definitely be planned, as it would improve fetch support and support for other APIs using AbortSignal, but I didn't add it at this time, as I was going for the minimal change to get cancellation working.

@benlesh benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label Jul 15, 2020
@benlesh
Copy link
Member Author

benlesh commented Jul 15, 2020

We really need to get the circle CI build stuff off of here. As it's no longer relevant. haha

voliva
voliva reviewed Jul 16, 2020
View reviewed changes
src/internal/Observable.ts Outdated Show resolved Hide resolved
@benlesh
chore: bump yet-another-abortcontroller-polyfill version
e819079 
adds event-target-polyfill, as the abortcontroller polyfill no longer polyfills EventTarget
cartant
cartant requested changes Jul 25, 2020
View reviewed changes
Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM. Noticed an unnecessary import that will need to be removed and isAbortError could/should be a user-defined type guard. And Victor's comment - https://github.com/ReactiveX/rxjs/pull/5591/files#r455752833 - needs to be addressed, too.

src/internal/Observable.ts Outdated Show resolved Hide resolved
spec/Observable-spec.ts Show resolved Hide resolved
src/internal/Observable.ts Outdated Show resolved Hide resolved
@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label Aug 12, 2020
@cartant cartant mentioned this pull request Aug 16, 2020
Merged
@benlesh
Copy link
Member Author

benlesh commented Aug 17, 2020

Related #5649

@benlesh benlesh requested a review from cartant August 23, 2020 21:41
This was referenced Oct 28, 2020
Closed
Closed
@benlesh
Copy link
Member Author

benlesh commented May 10, 2021

This is outdated, but remains on our radar. I'll close it for now.

@benlesh benlesh closed this May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@voliva voliva voliva left review comments

@cartant cartant Awaiting requested review from cartant

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benlesh @cartant @voliva