Skip to content

Commit 20e8b6b

Browse files
Juan Tejadafacebook-github-bot
Juan Tejada
authored and
facebook-github-bot
committedJan 14, 2021
Fix accidental cancelation of network request when using loadQuery apis
Reviewed By: josephsavona, rbalicki2 Differential Revision: D25860943 fbshipit-source-id: 3a2d31342910f9b1a487909ebbf2823af800c952
·
v18.2.0relay-compiler@experimental
1 parent 9cb374a commit 20e8b6b

File tree

4 files changed

+339
-41
lines changed

4 files changed

+339
-41
lines changed
 

‎packages/relay-experimental/__tests__/loadQuery-source-behavior-test.js

+104-1
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,16 @@ let writeDataToStore;
7575
let sink;
7676
let next;
7777
let error;
78+
let complete;
79+
let executeObservable;
80+
let executeUnsubscribe;
81+
let networkUnsubscribe;
7882

7983
beforeEach(() => {
84+
next = jest.fn();
85+
error = jest.fn();
86+
complete = jest.fn();
87+
8088
// In several tests, we expect unhandled errors from network requests
8189
// that emit errors after the query reference has been disposed.
8290
// The default behavior when encountering unhandled errors is to fail
@@ -94,15 +102,46 @@ beforeEach(() => {
94102
PreloadableQueryRegistry.clear();
95103

96104
fetch = jest.fn((_query, _variables, _cacheConfig) => {
97-
return Observable.create(_sink => {
105+
const observable = Observable.create(_sink => {
98106
sink = _sink;
99107
});
108+
const originalSubscribe = observable.subscribe.bind(observable);
109+
networkUnsubscribe = jest.fn();
110+
jest.spyOn(observable, 'subscribe').mockImplementation((...args) => {
111+
const subscription = originalSubscribe(...args);
112+
jest
113+
.spyOn(subscription, 'unsubscribe')
114+
.mockImplementation(() => networkUnsubscribe());
115+
return subscription;
116+
});
117+
return observable;
100118
});
101119

102120
environment = createMockEnvironment({network: Network.create(fetch)});
103121
const store = environment.getStore();
104122
const operation = createOperationDescriptor(query, variables);
105123

124+
const originalExecuteWithSource = environment.executeWithSource.getMockImplementation();
125+
executeObservable = undefined;
126+
executeUnsubscribe = undefined;
127+
128+
jest
129+
.spyOn(environment, 'executeWithSource')
130+
.mockImplementation((...params) => {
131+
executeObservable = originalExecuteWithSource(...params);
132+
const originalSubscribe = executeObservable.subscribe.bind(
133+
executeObservable,
134+
);
135+
jest
136+
.spyOn(executeObservable, 'subscribe')
137+
.mockImplementation(subscriptionCallbacks => {
138+
originalSubscribe(subscriptionCallbacks);
139+
executeUnsubscribe = jest.fn();
140+
return {unsubscribe: executeUnsubscribe};
141+
});
142+
return executeObservable;
143+
});
144+
106145
writeDataToStore = () => {
107146
loadQuery(environment, preloadableConcreteRequest, variables);
108147
sink.next(response);
@@ -129,10 +168,12 @@ beforeEach(() => {
129168

130169
next = jest.fn();
131170
error = jest.fn();
171+
complete = jest.fn();
132172
if (loadedQuery.source) {
133173
loadedQuery.source.subscribe({
134174
next,
135175
error,
176+
complete,
136177
});
137178
}
138179

@@ -351,6 +392,68 @@ describe('when passed a PreloadableConcreteRequest', () => {
351392
expect(error).not.toHaveBeenCalled();
352393
});
353394
});
395+
396+
describe('when loading and disposing same query multiple times', () => {
397+
it('loads correctly when ast is loaded in between calls to load and initial query ref is disposed', () => {
398+
// This test case simulates what happens in useQueryLoader or useEntryPointLoader, where the load
399+
// function can be called multiple times, and all previous query references corresponding to prior
400+
// calls to load will get disposed
401+
402+
// Start initial load of query
403+
const queryRef1 = loadQuery(
404+
environment,
405+
preloadableConcreteRequest,
406+
variables,
407+
{fetchPolicy: 'store-or-network'},
408+
);
409+
410+
// Assert that we start a network request, but we can't start
411+
// processing results (executeWithSource) since the query ast module
412+
// isn't available yet.
413+
expect(fetch).toHaveBeenCalledTimes(1);
414+
expect(environment.executeWithSource).toBeCalledTimes(0);
415+
416+
// Provide the query module ast
417+
PreloadableQueryRegistry.set(ID, query);
418+
// Assert that we are now able to start processing query results.
419+
expect(environment.executeWithSource).toBeCalledTimes(1);
420+
421+
// Start second load of query
422+
const queryRef2 = loadQuery(
423+
environment,
424+
preloadableConcreteRequest,
425+
variables,
426+
{fetchPolicy: 'store-or-network'},
427+
);
428+
429+
// Assert that we don't start a new network request or
430+
// processing execution, since they should be deduped with
431+
// the ones already in flight.
432+
expect(fetch).toHaveBeenCalledTimes(1);
433+
expect(environment.executeWithSource).toBeCalledTimes(1);
434+
435+
// Dispose of the initial query reference, like
436+
// useQueryLoader or useEntryPointLoader would
437+
queryRef1.dispose();
438+
expect(networkUnsubscribe).toHaveBeenCalledTimes(0);
439+
expect(executeUnsubscribe).toHaveBeenCalledTimes(0);
440+
441+
// Provide the initial response from the network
442+
sink.next(response);
443+
sink.complete();
444+
445+
// Subscribe to the query reference and assert that
446+
// we can observe the query results
447+
queryRef2.source?.subscribe({
448+
next,
449+
error,
450+
complete,
451+
});
452+
expect(next).toHaveBeenCalledTimes(1);
453+
expect(next).toHaveBeenCalledWith(response);
454+
expect(complete).toHaveBeenCalledTimes(1);
455+
});
456+
});
354457
});
355458
});
356459

‎packages/relay-experimental/__tests__/loadQuery-test.js

+5-2
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,11 @@ beforeEach(() => {
125125
jest
126126
.spyOn(executeObservable, 'subscribe')
127127
.mockImplementation(subscriptionCallbacks => {
128-
originalSubscribe(subscriptionCallbacks);
129-
executeUnsubscribe = jest.fn();
128+
const executeSubscription = originalSubscribe(subscriptionCallbacks);
129+
const originalUnsubscribe = executeSubscription.unsubscribe.bind(
130+
executeSubscription,
131+
);
132+
executeUnsubscribe = jest.fn(originalUnsubscribe);
130133
return {unsubscribe: executeUnsubscribe};
131134
});
132135
return executeObservable;

‎packages/relay-experimental/__tests__/useQueryLoader-multiple-calls-test.js

+180
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,180 @@
1+
/**
2+
* Copyright (c) Facebook, Inc. and its affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*
7+
* @emails oncall+relay
8+
* @flow strict-local
9+
* @format
10+
*/
11+
12+
// flowlint ambiguous-object-type:error
13+
14+
'use strict';
15+
16+
const React = require('react');
17+
const ReactTestRenderer = require('react-test-renderer');
18+
const RelayEnvironmentProvider = require('../RelayEnvironmentProvider');
19+
20+
const loadQueryModule = require('../loadQuery');
21+
const usePreloadedQuery = require('../usePreloadedQuery');
22+
const useQueryLoader = require('../useQueryLoader');
23+
24+
const {
25+
Network,
26+
Observable,
27+
PreloadableQueryRegistry,
28+
} = require('relay-runtime');
29+
const {
30+
generateAndCompile,
31+
createMockEnvironment,
32+
} = require('relay-test-utils-internal');
33+
34+
import type {ConcreteRequest} from 'relay-runtime';
35+
36+
const query: ConcreteRequest = generateAndCompile(`
37+
query TestQuery($id: ID!) {
38+
node(id: $id) {
39+
id
40+
}
41+
}
42+
`).TestQuery;
43+
44+
const preloadableConcreteRequest = {
45+
kind: 'PreloadableConcreteRequest',
46+
params: query.params,
47+
};
48+
49+
const response = {
50+
data: {
51+
node: {
52+
__typename: 'User',
53+
id: '4',
54+
},
55+
},
56+
extensions: {
57+
is_final: true,
58+
},
59+
};
60+
61+
// Only queries with an ID are preloadable
62+
const ID = '12345';
63+
(query.params: $FlowFixMe).id = ID;
64+
65+
const variables = {id: '4'};
66+
67+
let environment;
68+
let fetch;
69+
let sink;
70+
let executeObservable;
71+
let executeUnsubscribe;
72+
let networkUnsubscribe;
73+
74+
beforeEach(() => {
75+
jest.spyOn(loadQueryModule, 'loadQuery');
76+
77+
PreloadableQueryRegistry.clear();
78+
79+
fetch = jest.fn((_query, _variables, _cacheConfig) => {
80+
const observable = Observable.create(_sink => {
81+
sink = _sink;
82+
});
83+
const originalSubscribe = observable.subscribe.bind(observable);
84+
networkUnsubscribe = jest.fn();
85+
jest.spyOn(observable, 'subscribe').mockImplementation((...args) => {
86+
const subscription = originalSubscribe(...args);
87+
jest
88+
.spyOn(subscription, 'unsubscribe')
89+
.mockImplementation(() => networkUnsubscribe());
90+
return subscription;
91+
});
92+
return observable;
93+
});
94+
95+
environment = createMockEnvironment({network: Network.create(fetch)});
96+
97+
const originalExecuteWithSource = environment.executeWithSource.getMockImplementation();
98+
executeObservable = undefined;
99+
executeUnsubscribe = undefined;
100+
101+
jest
102+
.spyOn(environment, 'executeWithSource')
103+
.mockImplementation((...params) => {
104+
executeObservable = originalExecuteWithSource(...params);
105+
const originalSubscribe = executeObservable.subscribe.bind(
106+
executeObservable,
107+
);
108+
jest
109+
.spyOn(executeObservable, 'subscribe')
110+
.mockImplementation(subscriptionCallbacks => {
111+
originalSubscribe(subscriptionCallbacks);
112+
executeUnsubscribe = jest.fn();
113+
return {unsubscribe: executeUnsubscribe};
114+
});
115+
return executeObservable;
116+
});
117+
});
118+
119+
describe('when loading and disposing same query multiple times', () => {
120+
it('loads correctly when ast is loaded in between calls to load and initial query ref is disposed', () => {
121+
let loadedQuery;
122+
let queryLoaderCallback;
123+
124+
const QueryRenderer = function({queryRef}) {
125+
const data = usePreloadedQuery(query, queryRef);
126+
return data.node.id;
127+
};
128+
const Inner = function({initialPreloadedQuery}) {
129+
[loadedQuery, queryLoaderCallback] = useQueryLoader(
130+
preloadableConcreteRequest,
131+
// $FlowExpectedError[incompatible-call] it's ok to pass our fake preloaded query here
132+
initialPreloadedQuery,
133+
);
134+
return (
135+
<React.Suspense fallback="Loading">
136+
{loadedQuery != null ? (
137+
<QueryRenderer queryRef={loadedQuery} />
138+
) : null}
139+
</React.Suspense>
140+
);
141+
};
142+
const Container = function({initialPreloadedQuery = undefined}) {
143+
return (
144+
<RelayEnvironmentProvider environment={environment}>
145+
<Inner initialPreloadedQuery={initialPreloadedQuery} />
146+
</RelayEnvironmentProvider>
147+
);
148+
};
149+
let instance;
150+
const render = () => {
151+
ReactTestRenderer.act(() => {
152+
instance = ReactTestRenderer.create(
153+
<RelayEnvironmentProvider environment={environment}>
154+
<Container />
155+
</RelayEnvironmentProvider>,
156+
);
157+
});
158+
};
159+
render();
160+
if (!instance) {
161+
throw new Error('Expected renderer instance to be defined');
162+
}
163+
164+
ReactTestRenderer.act(() => {
165+
queryLoaderCallback(variables);
166+
// Provide the query module ast
167+
PreloadableQueryRegistry.set(ID, query);
168+
queryLoaderCallback(variables);
169+
});
170+
expect(instance.toJSON()).toEqual('Loading');
171+
172+
ReactTestRenderer.act(() => {
173+
sink.next(response);
174+
sink.complete();
175+
jest.runAllImmediates();
176+
});
177+
178+
expect(instance.toJSON()).toEqual('4');
179+
});
180+
});

‎packages/relay-experimental/loadQuery.js

+50-38
Original file line numberDiff line numberDiff line change
@@ -84,21 +84,52 @@ function loadQuery<TQuery: OperationType, TEnvironmentProviderOptions>(
8484
force: true,
8585
};
8686

87+
// executeWithNetworkSource will retain and execute an operation
88+
// against the Relay store, given an Observable that would provide
89+
// the network events for the operation.
90+
let retainReference;
91+
let didExecuteNetworkSource = false;
92+
const executeWithNetworkSource = (
93+
operation: OperationDescriptor,
94+
networkObservable: Observable<GraphQLResponse>,
95+
): Observable<GraphQLResponse> => {
96+
didExecuteNetworkSource = true;
97+
retainReference = environment.retain(operation);
98+
return environment.executeWithSource({
99+
operation,
100+
source: networkObservable,
101+
});
102+
};
103+
104+
// N.B. For loadQuery, we unconventionally want to return an Observable
105+
// that isn't lazily executed, meaning that we don't want to wait
106+
// until the returned Observable is subscribed to to actually start
107+
// fetching and executing an operation; i.e. we want to execute the
108+
// operation eagerly, when loadQuery is called.
109+
// For this reason, we use an intermediate executionSubject which
110+
// allows us to capture the events that occur during the eager execution
111+
// of the operation, and then replay them to the Observable we
112+
// ultimately return.
113+
const executionSubject = new ReplaySubject();
114+
const returnedObservable = Observable.create(sink =>
115+
executionSubject.subscribe(sink),
116+
);
117+
87118
let unsubscribeFromNetworkRequest;
88119
let networkError = null;
89120
// makeNetworkRequest will immediately start a raw network request if
90121
// one isn't already in flight and return an Observable that when
91122
// subscribed to will replay the network events that have occured so far,
92123
// as well as subsequent events.
93-
let madeNetworkRequest = false;
124+
let didMakeNetworkRequest = false;
94125
const makeNetworkRequest = (params): Observable<GraphQLResponse> => {
95126
// N.B. this function is called synchronously or not at all
96-
// madeNetworkRequest is safe to rely on in the returned value
127+
// didMakeNetworkRequest is safe to rely on in the returned value
97128
// Even if the request gets deduped below, we still wan't to return an
98129
// observable that provides the replayed network events for the query,
99130
// so we set this to true before deduping, to guarantee that the
100131
// `source` observable is returned.
101-
madeNetworkRequest = true;
132+
didMakeNetworkRequest = true;
102133

103134
let observable;
104135
const subject = new ReplaySubject();
@@ -129,7 +160,7 @@ function loadQuery<TQuery: OperationType, TEnvironmentProviderOptions>(
129160
observable = network.execute(params, variables, networkCacheConfig);
130161
}
131162

132-
({unsubscribe: unsubscribeFromNetworkRequest} = observable.subscribe({
163+
const {unsubscribe} = observable.subscribe({
133164
error(err) {
134165
networkError = err;
135166
subject.error(err);
@@ -140,39 +171,17 @@ function loadQuery<TQuery: OperationType, TEnvironmentProviderOptions>(
140171
complete() {
141172
subject.complete();
142173
},
143-
}));
144-
return Observable.create(sink => subject.subscribe(sink));
145-
};
146-
147-
// executeWithNetworkSource will retain and execute an operation
148-
// against the Relay store, given an Observable that would provide
149-
// the network events for the operation.
150-
let retainReference;
151-
const executeWithNetworkSource = (
152-
operation: OperationDescriptor,
153-
networkObservable: Observable<GraphQLResponse>,
154-
): Observable<GraphQLResponse> => {
155-
retainReference = environment.retain(operation);
156-
return environment.executeWithSource({
157-
operation,
158-
source: networkObservable,
174+
});
175+
unsubscribeFromNetworkRequest = unsubscribe;
176+
return Observable.create(sink => {
177+
const subjectSubscription = subject.subscribe(sink);
178+
return () => {
179+
subjectSubscription.unsubscribe();
180+
unsubscribeFromNetworkRequest();
181+
};
159182
});
160183
};
161184

162-
// N.B. For loadQuery, we unconventionally want to return an Observable
163-
// that isn't lazily executed, meaning that we don't want to wait
164-
// until the returned Observable is subscribed to to actually start
165-
// fetching and executing an operation; i.e. we want to execute the
166-
// operation eagerly, when loadQuery is called.
167-
// For this reason, we use an intermediate executionSubject which
168-
// allows us to capture the events that occur during the eager execution
169-
// of the operation, and then replay them to the Observable we
170-
// ultimately return.
171-
const executionSubject = new ReplaySubject();
172-
const returnedObservable = Observable.create(sink =>
173-
executionSubject.subscribe(sink),
174-
);
175-
176185
let unsubscribeFromExecution;
177186
const executeDeduped = (
178187
operation: OperationDescriptor,
@@ -187,7 +196,7 @@ function loadQuery<TQuery: OperationType, TEnvironmentProviderOptions>(
187196
// an observable that provides the replayed network events for the query,
188197
// so we set this to true before deduping, to guarantee that the `source`
189198
// observable is returned.
190-
madeNetworkRequest = true;
199+
didMakeNetworkRequest = true;
191200
}
192201

193202
// Here, we are calling fetchQueryDeduped, which ensures that only
@@ -301,8 +310,11 @@ function loadQuery<TQuery: OperationType, TEnvironmentProviderOptions>(
301310
if (isDisposed) {
302311
return;
303312
}
304-
unsubscribeFromNetworkRequest && unsubscribeFromNetworkRequest();
305-
unsubscribeFromExecution && unsubscribeFromExecution();
313+
if (didExecuteNetworkSource) {
314+
unsubscribeFromExecution && unsubscribeFromExecution();
315+
} else {
316+
unsubscribeFromNetworkRequest && unsubscribeFromNetworkRequest();
317+
}
306318
retainReference && retainReference.dispose();
307319
cancelOnLoadCallback && cancelOnLoadCallback();
308320
isDisposed = true;
@@ -320,7 +332,7 @@ function loadQuery<TQuery: OperationType, TEnvironmentProviderOptions>(
320332
name: params.name,
321333
networkCacheConfig,
322334
fetchPolicy,
323-
source: madeNetworkRequest ? returnedObservable : undefined,
335+
source: didMakeNetworkRequest ? returnedObservable : undefined,
324336
variables,
325337
};
326338
}

0 commit comments

Comments
 (0)
Please sign in to comment.