feat(fetch): add selector

[cartant]

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]

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]

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? 🙂

[felixfbecker]

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

[cartant]

Done

[felixfbecker]

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

[benlesh]

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.

[cartant]

@benlesh

Perhaps just add a selector property to RequestInit?

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

[felixfbecker]

Ship it ✨

[benlesh]

Action Items

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

[cartant]

@benlesh Done. See new commits.

[iamandrewluca]

@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

[cartant]

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