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

fix(fromFetch): don't abort if fetch resolves #4742

Merged
merged 2 commits into from May 2, 2019

Conversation

@cartant
Copy link
Collaborator

commented Apr 26, 2019

Description:

This PR rearranges an existing test so that the expectations are performed after the complete - and subsequent unsubscribe - are finished. The rearranged test fails.

It also adds a flag to the fromFetch implementation so that abort is not called in the teardown if the promise returned by fetch has resolved.

This fixes the problem, but it also highlights a problem with the implementation: streamed data obtained using fromFetch cannot be aborted, as the AbortController passed to fetch also aborts the promises returned from text and json, etc.

Related issue (if exists): #4739

@cartant cartant requested a review from benlesh Apr 26, 2019
Closes #4739
@cartant cartant force-pushed the cartant:issue-4739 branch from b13d419 to ed8d771 Apr 26, 2019
@coveralls

This comment has been minimized.

Copy link

commented Apr 26, 2019

Pull Request Test Coverage Report for Build 8424

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 96.677%

Files with Coverage Reduction New Missed Lines %
src/internal/OuterSubscriber.ts 1 50.0%
Totals Coverage Status
Change from base Build 8414: -0.2%
Covered Lines: 5208
Relevant Lines: 5387

💛 - Coveralls
@benlesh
benlesh approved these changes May 2, 2019
@benlesh benlesh merged commit 1dc09e9 into ReactiveX:master May 2, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.2%) to 96.677%
Details
@benlesh

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Thanks for picking this one up @cartant!

@topaxi

This comment has been minimized.

Copy link

commented May 9, 2019

Just ran into this issue, any chance for a release?

BioPhoton added a commit to BioPhoton/rxjs that referenced this pull request May 15, 2019
* test(fetch): test expectations after complete

* fix(fromFetch): don't abort if fetch resolves

Closes ReactiveX#4739
@lock lock bot locked as resolved and limited conversation to collaborators Jun 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.