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

feat(fetch): add selector #5306

Merged
merged 6 commits into from
Apr 22, 2020
Merged

feat(fetch): add selector #5306

merged 6 commits into from
Apr 22, 2020

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented Feb 9, 2020

Description:

This PR adds an optional selector to fromFetch so that it's better able to support chunked transfer encoding.

When chunked transfer encoding is used, the headers are received and the Response is resolved before the entire content is received. Without a using selector, it's not possible to abort the promises returned from the Response's methods - text, json, blob, et al.

const mapped = fromFetch('/foo').pipe(
  mergeMap(res => res.text())
);

In the above snippet, once the headers are received, the Response will be emitted and mapped to the text() promise. Whilst the chunked content is being received, the fromFetch will complete, so explicit unsubscription from the mapped observable will not abort the text() promise and all of the content will be retrieved.

const selected = fromFetch('/foo', undefined, res => res.text());

When a selector is used - as above - explicit unsubscription from the selected observable will abort the text() promise and the browser will stop the request and will not download further data.

For more information, see the linked issue - #4744 - and I've created a harness here if anyone wants to play around with this stuff: https://github.com/cartant/rxjs-issue-4744

Also, this PR includes some of the changes made in #5305. I'll rebase this once/if that PR is merged.

Related issue: #4744

@cartant
Copy link
Collaborator Author

cartant commented Feb 9, 2020

BTW, this actually addresses a bug, IMO. Unless a selector is used, a signal that's passed in - via the init object - won't always abort the text() (or json(), et al.) promises for chunked responses.

@felixfbecker
Copy link
Contributor

We faced the bug in Sourcegraph too and I can confirm this PR fixes it and everything else still works. Any chance we got get this merged soon? 🙂

export function fromFetch<T>(
input: string | Request,
init: RequestInit | undefined,
selector: (response: Response) => ObservableInput<T>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at most of our uses, an overload for fromFetch(input, selector) without init would be useful (but not necessary)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@felixfbecker
Copy link
Contributor

The upgrade 6.5.3 -> 6.5.4 was actually failing tests for us, but they pass after this PR

Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

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

I would like to have this use a configuration argument instead of 3 separate arguments.

Perhaps just add a selector property to RequestInit? RequestInit & { selector: (response: Response) => ObservableInput<T> }

fromFetch('url/here', {
  selector: res => res.json()
});

I just don't like more than 2 arguments without names associated with them.

@benlesh benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label Apr 3, 2020
@cartant
Copy link
Collaborator Author

cartant commented Apr 3, 2020

@benlesh

Perhaps just add a selector property to RequestInit?

Done. And the implementation is nicer, too. Thanks.

Copy link
Contributor

@felixfbecker felixfbecker left a comment

Choose a reason for hiding this comment

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

Ship it ✨

@benlesh
Copy link
Member

benlesh commented Apr 8, 2020

Action Items

  • Documentation for chunky bodies :p (just a blurb or something with advice and the "why", maybe?)
  • dtslint test for selector case.

@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label Apr 8, 2020
@cartant
Copy link
Collaborator Author

cartant commented Apr 19, 2020

@benlesh Done. See new commits.

@benlesh benlesh merged commit 99b5af1 into ReactiveX:master Apr 22, 2020
cartant added a commit to cartant/rxjs that referenced this pull request May 17, 2020
* feat(fetch): add selector
benlesh pushed a commit that referenced this pull request May 18, 2020
* feat(fetch): add selector (#5306)

* feat(fetch): add selector

* chore: fix import in dtslint test

* chore: update side-effect snapshots
@cartant cartant deleted the issue-4744 branch September 24, 2020 07:09
selector?: (response: Response) => ObservableInput<T>
} = {}
): Observable<Response | T> {
const { selector, ...init } = initWithSelector;
Copy link
Contributor

@iamandrewluca iamandrewluca Dec 14, 2020

Choose a reason for hiding this comment

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

@benlesh @cartant An observation. Now that init is created using spread, this means that init always will be an object, even it has no fields. And below wrap with if(init) has no value

https://github.com/ReactiveX/rxjs/pull/5306/files#diff-717ea4ff7feb7c6009e9048143f3ef0fc9ddc6cdeb16b1effcb19d18f3a9c875R119

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@iamandrewluca 👍 Yep. Feel free to open a PR to simplify it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants