Skip to content

Commit

Permalink
Make sure loadQuery requests are deduped when started (vs at render t…
Browse files Browse the repository at this point in the history
…ime)

Reviewed By: josephsavona

Differential Revision: D23171834

fbshipit-source-id: e368f913150ea1aecac92cd66c0d7083635671a2
  • Loading branch information
Juan Tejada authored and facebook-github-bot committed Aug 21, 2020
1 parent 0c31374 commit a21b1cb
Show file tree
Hide file tree
Showing 6 changed files with 153 additions and 76 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ const ID = '12345';
const variables = {id: '4'};

let callLoadQuery;
let environment;
let fetch;
let writeDataToStore;
let sink;
let next;
Expand All @@ -77,19 +79,20 @@ let error;
beforeEach(() => {
PreloadableQueryRegistry.clear();

const fetch = jest.fn((_query, _variables, _cacheConfig) => {
fetch = jest.fn((_query, _variables, _cacheConfig) => {
return Observable.create(_sink => {
sink = _sink;
});
});

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

writeDataToStore = () => {
loadQuery(environment, preloadableConcreteRequest, variables);
sink.next(response);
sink.complete();
PreloadableQueryRegistry.set(ID, query);
expect(store.check(operation).status).toBe('available');
// N.B. we are not testing the case where data is written to the store
Expand Down Expand Up @@ -134,6 +137,14 @@ describe('when passed a PreloadableConcreteRequest', () => {
expect(next).toHaveBeenCalledWith(response);
});

it('should dedupe network request if called multiple times', () => {
PreloadableQueryRegistry.set(ID, query);
callLoadQuery(preloadableConcreteRequest);
callLoadQuery(preloadableConcreteRequest);

expect(fetch).toHaveBeenCalledTimes(1);
});

it('should pass network errors onto source', () => {
PreloadableQueryRegistry.set(ID, query);
callLoadQuery(preloadableConcreteRequest);
Expand Down Expand Up @@ -164,6 +175,22 @@ describe('when passed a PreloadableConcreteRequest', () => {
});

describe('when the query is unavailable synchronously', () => {
it('should dedupe operation execution if called multiple times', () => {
callLoadQuery(preloadableConcreteRequest);
callLoadQuery(preloadableConcreteRequest);

// Note: before we have the operation module is available
// we can't reliably dedupe network requests, since the
// request identifier is based on the variables the
// operation expects, and not just the variables passed as
// input.
expect(fetch).toHaveBeenCalledTimes(2);

PreloadableQueryRegistry.set(ID, query);
// We only process the network request once.
expect(environment.executeWithSource).toBeCalledTimes(1);
});

describe('when the query AST is available before the network response', () => {
it('should pass network responses onto source', () => {
callLoadQuery(preloadableConcreteRequest);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ beforeEach(() => {
writeDataToStore = () => {
loadQuery(environment, preloadableConcreteRequest, variables);
sink.next(response);
sink.complete();
PreloadableQueryRegistry.set(ID, query);
expect(store.check(operation).status).toBe('available');
const snapshot: $FlowFixMe = store.lookup(operation.fragment);
Expand Down
5 changes: 2 additions & 3 deletions packages/relay-experimental/__tests__/loadQuery-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,10 @@ beforeEach(() => {
return {dispose: disposeOnloadCallback};
});

const originalExecuteWithSource = environment.executeWithSource.bind(
environment,
);
const originalExecuteWithSource = environment.executeWithSource.getMockImplementation();
executeObservable = undefined;
executeUnsubscribe = undefined;

jest
.spyOn(environment, 'executeWithSource')
.mockImplementation((...params) => {
Expand Down
124 changes: 89 additions & 35 deletions packages/relay-experimental/loadQuery.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const {
createOperationDescriptor,
getRequest,
Observable,
__internal: {fetchQueryDeduped},
} = require('relay-runtime');

import type {
Expand All @@ -30,6 +31,7 @@ import type {
} from './EntryPointTypes.flow';
import type {
IEnvironment,
OperationDescriptor,
OperationType,
GraphQLTaggedNode,
GraphQLResponse,
Expand Down Expand Up @@ -57,8 +59,7 @@ function loadQuery<TQuery: OperationType, TEnvironmentProviderOptions>(
options?: LoadQueryOptions,
environmentProviderOptions?: TEnvironmentProviderOptions,
): PreloadedQueryInner<TQuery, TEnvironmentProviderOptions> {
// Flow does not know of React internals (rightly so), but we need to
// ensure here that this function isn't called inside render.
// This code ensures that we don't call loadQuery during render.
const CurrentDispatcher =
// $FlowFixMe[prop-missing]
React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED
Expand All @@ -74,6 +75,9 @@ function loadQuery<TQuery: OperationType, TEnvironmentProviderOptions>(
force: true,
};

// makeNetworkRequest will immediately start a raw network request and
// return an Observable that when subscribing to it, will replay the
// network events that have occured so far, as well as subsequent events.
let madeNetworkRequest = false;
const makeNetworkRequest = (params): Observable<GraphQLResponse> => {
// N.B. this function is called synchronously or not at all
Expand Down Expand Up @@ -102,45 +106,90 @@ function loadQuery<TQuery: OperationType, TEnvironmentProviderOptions>(
return Observable.create(sink => subject.subscribe(sink));
};

const normalizationSubject = new ReplaySubject();
// executeWithNetworkSource will retain and execute an operation
// against the Relay store, given an Observable that would provide
// the network events for the operation.
let retainReference;
const executeWithNetworkSource = (
operation: OperationDescriptor,
networkObservable: Observable<GraphQLResponse>,
): Observable<GraphQLResponse> => {
retainReference = environment.retain(operation);
return environment.executeWithSource({
operation,
source: networkObservable,
});
};

// N.B. For loadQuery, we unconventionally want to return an Observable
// that isn't lazily executed, meaning that we don't want to wait
// until the returned Observable is subscribed to to actually start
// fetching and executing an operation; i.e. we want to execute the
// operation eagerly, when loadQuery is called.
// For this reason, we use an intermediate executionSubject which
// allows us to capture the events that occur during the eager execution
// of the operation, and then replay them to the Observable we
// ultimately return.
const executionSubject = new ReplaySubject();
const returnedObservable = Observable.create(sink =>
normalizationSubject.subscribe(sink),
executionSubject.subscribe(sink),
);

let unsubscribeFromExecute;
let retainReference;
const executeWithSource = (operation, sourceObservable) => {
retainReference = environment.retain(operation);
({unsubscribe: unsubscribeFromExecute} = environment
.executeWithSource({
operation,
source: sourceObservable,
})
.subscribe({
error(err) {
normalizationSubject.error(err);
},
next(data) {
normalizationSubject.next(data);
},
complete() {
normalizationSubject.complete();
},
}));
let unsubscribeFromExecution;
const executeDeduped = (
operation: OperationDescriptor,
fetchFn: () => Observable<GraphQLResponse>,
) => {
// N.B.
// Here, we are calling fetchQueryDeduped, which ensures that only
// a single operation is active for a given (environment, identifier) pair,
// and also tracks the active state of the operation, which is necessary
// for our Suspense infra to later be able to suspend (or not) on
// active operations.
// - If a duplicate active operation is found, it will return an
// Observable that replays the events of the already active operation.
// - If no duplicate active operation is found, it will call the fetchFn
// to execute the operation, and return an Observable that will provide
// the events for executing the operation.
({unsubscribe: unsubscribeFromExecution} = fetchQueryDeduped(
environment,
operation.request.identifier,
fetchFn,
).subscribe({
error(err) {
executionSubject.error(err);
},
next(data) {
executionSubject.next(data);
},
complete() {
executionSubject.complete();
},
}));
};

const checkAvailabilityAndExecute = concreteRequest => {
const operation = createOperationDescriptor(concreteRequest, variables);

// N.B. If the fetch policy allows fulfillment from the store but the
// environment already has the data for that operation cached in the store,
// then we do nothing.
const shouldFetch =
fetchPolicy !== 'store-or-network' ||
environment.check(operation).status !== 'available';

if (shouldFetch) {
const source = makeNetworkRequest(concreteRequest.params);
executeWithSource(operation, source);
executeDeduped(operation, () => {
// N.B. Since we have the operation synchronously available here,
// we can immediately fetch and execute the operation.
const networkObservable = makeNetworkRequest(concreteRequest.params);
const executeObservable = executeWithNetworkSource(
operation,
networkObservable,
);
return executeObservable;
});
}
// if the fetch policy allows fulfillment from the store and the environment
// has the appropriate data, we do nothing.
};

let params;
Expand All @@ -163,9 +212,13 @@ function loadQuery<TQuery: OperationType, TEnvironmentProviderOptions>(
if (module != null) {
checkAvailabilityAndExecute(module);
} else {
// If the module isn't synchronously available, we launch the network request
// immediately and ignore the fetch policy.
const source = makeNetworkRequest(params);
// If the module isn't synchronously available, we launch the
// network request immediately and ignore the fetch policy.
// Note that without the operation module we can't reliably
// dedupe network requests, since the request identifier is
// based on the variables the operation expects, and not
// just the variables passed as input.
const networkObservable = makeNetworkRequest(params);
({dispose: cancelOnLoadCallback} = PreloadableQueryRegistry.onLoad(
moduleId,
preloadedModule => {
Expand All @@ -175,7 +228,9 @@ function loadQuery<TQuery: OperationType, TEnvironmentProviderOptions>(
preloadedModule,
variables,
);
executeWithSource(operation, source);
executeDeduped(operation, () =>
executeWithNetworkSource(operation, networkObservable),
);
},
));
if (!environment.isServer()) {
Expand All @@ -187,15 +242,14 @@ function loadQuery<TQuery: OperationType, TEnvironmentProviderOptions>(
}
// complete() the subject so that the observer knows no (additional) payloads
// will be delivered
normalizationSubject.complete();
executionSubject.complete();
}, LOAD_QUERY_AST_MAX_TIMEOUT);
}
}
} else {
const graphQlTaggedNode: GraphQLTaggedNode = (preloadableRequest: $FlowFixMe);
const request = getRequest(graphQlTaggedNode);
params = request.params;

checkAvailabilityAndExecute(request);
}

Expand All @@ -208,7 +262,7 @@ function loadQuery<TQuery: OperationType, TEnvironmentProviderOptions>(
if (isDisposed) {
return;
}
unsubscribeFromExecute && unsubscribeFromExecute();
unsubscribeFromExecution && unsubscribeFromExecution();
retainReference && retainReference.dispose();
cancelOnLoadCallback && cancelOnLoadCallback();
loadQueryAstTimeoutId != null && clearTimeout(loadQueryAstTimeoutId);
Expand Down
67 changes: 31 additions & 36 deletions packages/relay-experimental/usePreloadedQuery.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ const {useTrackLoadQueryInRender} = require('./loadQuery');
const {useDebugValue} = require('react');
const {
__internal: {fetchQueryDeduped},
Observable,
} = require('relay-runtime');

import type {PreloadedQuery} from './EntryPointTypes.flow';
Expand Down Expand Up @@ -85,47 +84,43 @@ function usePreloadedQuery<TQuery: OperationType>(
'collection, and as such query results may no longer be present in the Relay ' +
'store. In the future, this will become a hard error.',
);
// Here, we are calling fetchQueryDeduped, which usually ensures that only
// a single network request is active for a given (environment, identifier) pair.
// If no network request is active, it will call the third argument and initiate
// a network request.
//
// However, if preloadedQuery.kind === 'PreloadedQuery', the network request (if
// it exists) has already been made.
//
// Thus, if two calls to loadQuery are made with the same environment and identifier
// (i.e. the same request is made twice), the second query will be deduped
// and components will suspend for the duration of the first query.
const dedupedSource = fetchQueryDeduped(

const fallbackFetchObservable = fetchQueryDeduped(
environment,
operation.request.identifier,
() => {
if (source && environment === preloadedQuery.environment) {
return source.ifEmpty(
Observable.create(sink => {
return environment.execute({operation}).subscribe(sink);
}),
);
} else {
// if a call to loadQuery is made with a particular environment, and that
// preloaded query is passed to usePreloadedQuery in a different environmental
// context, we cannot re-use the existing preloaded query. Instead, we must
// re-execute the query with the new environment (at render time.)
// TODO T68036756 track occurences of this warning and turn it into a hard error
warning(
false,
'usePreloadedQuery(): usePreloadedQuery was passed a preloaded query ' +
'that was created with a different environment than the one that is currently ' +
'in context. In the future, this will become a hard error.',
);
return environment.execute({operation});
}
},
() => environment.execute({operation}),
);
let fetchObservable;
if (source != null && environment === preloadedQuery.environment) {
// If the source observable exists and the environments match, reuse
// the source observable.
// If the source observable happens to be empty, we need to fall back
// and re-execute and de-dupe the query (at render time).
fetchObservable = source.ifEmpty(fallbackFetchObservable);
} else if (environment !== preloadedQuery.environment) {
// If a call to loadQuery is made with a particular environment, and that
// preloaded query is passed to usePreloadedQuery in a different environment
// context, we cannot re-use the existing preloaded query.
// Instead, we need to fall back and re-execute and de-dupe the query with
// the new environment (at render time).
// TODO T68036756 track occurences of this warning and turn it into a hard error
warning(
false,
'usePreloadedQuery(): usePreloadedQuery was passed a preloaded query ' +
'that was created with a different environment than the one that is currently ' +
'in context. In the future, this will become a hard error.',
);
fetchObservable = fallbackFetchObservable;
} else {
// if (source == null)
// If the source observable does not exist, we need to
// fall back and re-execute and de-dupe the query (at render time).
fetchObservable = fallbackFetchObservable;
}

useLazyLoadQueryNodeParams = {
componentDisplayName: 'usePreloadedQuery()',
fetchObservable: dedupedSource,
fetchObservable,
fetchPolicy,
query: operation,
renderPolicy: options?.UNSTABLE_renderPolicy,
Expand Down
1 change: 1 addition & 0 deletions packages/relay-test-utils/RelayModernMockEnvironment.js
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,7 @@ function createMockEnvironment(
mockDisposableMethod(environment, 'subscribe');
mockDisposableMethod(environment, 'retain');
mockObservableMethod(environment, 'execute');
mockObservableMethod(environment, 'executeWithSource');
mockObservableMethod(environment, 'executeMutation');

mockInstanceMethod(store, 'getSource');
Expand Down

0 comments on commit a21b1cb

Please sign in to comment.