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

Reevaluate window.fetch each time HttpLink uses it, if not configured using options.fetch #8603

Merged
merged 3 commits into from
Aug 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
## Apollo Client 3.4.6 (not yet released)

## Improvements

- Reevaluate `window.fetch` each time `HttpLink` uses it, if not configured using `options.fetch`. This change enables a variety of strategies for instrumenting `window.fetch`, without requiring those strategies to run before `@apollo/client/link/http` is first imported. <br/>
[@benjamn](https://github.com/benjamn) in [#8603](https://github.com/apollographql/apollo-client/pull/8603)

## Apollo Client 3.4.5

### Bug Fixes
Expand Down
1 change: 1 addition & 0 deletions src/__tests__/__snapshots__/exports.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,7 @@ Array [
"iterateObserversSafely",
"makeReference",
"makeUniqueId",
"maybe",
"maybeDeepFreeze",
"mergeDeep",
"mergeDeepArray",
Expand Down
55 changes: 55 additions & 0 deletions src/link/http/__tests__/HttpLink.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ describe('HttpLink', () => {
const data2 = { data: { hello: 'everyone' } };
const mockError = { throws: new TypeError('mock me') };
let subscriber: ZenObservable.Observer<any>;
const subscriptions = new Set<ZenObservable.Subscription>();

beforeEach(() => {
fetchMock.restore();
Expand All @@ -74,10 +75,17 @@ describe('HttpLink', () => {
error,
complete,
};

subscriptions.clear();
});

afterEach(() => {
fetchMock.restore();
if (subscriptions.size) {
// Tests within this describe block can add subscriptions to this Set
// that they want to be canceled/unsubscribed after the test finishes.
subscriptions.forEach(sub => sub.unsubscribe());
}
});

it('does not need any constructor arguments', () => {
Expand Down Expand Up @@ -830,6 +838,53 @@ describe('HttpLink', () => {
);
});

it('uses the latest window.fetch function if options.fetch not configured', done => {
const httpLink = createHttpLink({ uri: 'data' });

const fetch = window.fetch;
expect(typeof fetch).toBe('function');

const fetchSpy = jest.spyOn(window, 'fetch');
fetchSpy.mockImplementation(() => Promise.resolve<Response>({
text() {
return Promise.resolve(JSON.stringify({
data: { hello: "from spy" },
}));
},
} as Response));

const spyFn = window.fetch;
expect(spyFn).not.toBe(fetch);

subscriptions.add(execute(httpLink, {
query: sampleQuery,
}).subscribe({
error: done.fail,

next(result) {
expect(fetchSpy).toHaveBeenCalledTimes(1);
expect(result).toEqual({
data: { hello: "from spy" },
});

fetchSpy.mockRestore();
expect(window.fetch).toBe(fetch);

subscriptions.add(execute(httpLink, {
query: sampleQuery,
}).subscribe({
error: done.fail,
next(result) {
expect(result).toEqual({
data: { hello: "world" },
});
done();
},
}));
},
}));
});

it('prioritizes context over setup', done => {
const variables = { params: 'stub' };
const middleware = new ApolloLink((operation, forward) => {
Expand Down
26 changes: 16 additions & 10 deletions src/link/http/createHttpLink.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,26 +14,25 @@ import {
import { createSignalIfSupported } from './createSignalIfSupported';
import { rewriteURIForGET } from './rewriteURIForGET';
import { fromError } from '../utils';
import { maybe } from '../../utilities';

const backupFetch = maybe(() => fetch);

export const createHttpLink = (linkOptions: HttpOptions = {}) => {
let {
uri = '/graphql',
// use default global fetch if nothing passed in
fetch: fetcher,
fetch: preferredFetch,
includeExtensions,
useGETForQueries,
includeUnusedVariables = false,
...requestOptions
} = linkOptions;

// dev warnings to ensure fetch is present
checkFetcher(fetcher);

//fetcher is set here rather than the destructuring to ensure fetch is
//declared before referencing it. Reference in the destructuring would cause
//a ReferenceError
if (!fetcher) {
fetcher = fetch;
if (__DEV__) {
// Make sure at least one of preferredFetch, window.fetch, or backupFetch is
// defined, so requests won't fail at runtime.
checkFetcher(preferredFetch || backupFetch);
}

const linkConfig = {
Expand Down Expand Up @@ -142,7 +141,14 @@ export const createHttpLink = (linkOptions: HttpOptions = {}) => {
}

return new Observable(observer => {
fetcher!(chosenURI, options)
// Prefer linkOptions.fetch (preferredFetch) if provided, and otherwise
// fall back to the *current* global window.fetch function (see issue
// #7832), or (if all else fails) the backupFetch function we saved when
// this module was first evaluated. This last option protects against the
// removal of window.fetch, which is unlikely but not impossible.
const currentFetch = preferredFetch || maybe(() => fetch) || backupFetch;
Copy link

Choose a reason for hiding this comment

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

Question: There should no runtime difference between maybe(() => fetch) and backupFetch, as backupFetch is also maybe(() => fetch). In both cases the fetch variable is resolved during execution time of the maybe lambda. What is reason for the backupFetch fallback / the extra maybe(() => fetch)?

What is the reason for backupFetch here?

Suggested change
const currentFetch = preferredFetch || maybe(() => fetch) || backupFetch;
const currentFetch = preferredFetch || backupFetch;

Copy link
Member Author

Choose a reason for hiding this comment

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

@bripkens A maybe(() => fetch) expression executes the () => fetch function immediately and returns the current result, or undefined if the function threw an exception. It's the shortest way I've found to evaluate possibly-undefined global variables without obtaining a reference to the global object, and TypeScript infers the return type reliably.

To my way of thinking, this means the maybe(() => fetch) expression we evaluate here could be different from backupFetch, which was evaluated when the module was first imported (possibly long ago). I think this difference allows window.fetch to be wrapped after the HttpLink is created (the goal of #7832, IIUC). If we switch to preferredFetch || backupFetch here, then we're stuck with one of those values for all time.

Copy link

Choose a reason for hiding this comment

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

Oh, my bad, sorry @benjamn. I misunderstood what maybe is doing! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

A typeof fetch === "undefined" check might be clearer, but that’s a nit. I don’t like this maybe() function or relying on thrown ReferenceError in a callback, but I can tolerate it.


currentFetch!(chosenURI, options)
.then(response => {
operation.setContext({ response });
return response;
Expand Down
1 change: 1 addition & 0 deletions src/utilities/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ export {
export * from './common/mergeDeep';
export * from './common/cloneDeep';
export * from './common/maybeDeepFreeze';
export * from './common/maybe';
export * from './observables/iteration';
export * from './observables/asyncMap';
export * from './observables/Concast';
Expand Down