Make sure loadQuery requests are deduped when started (vs at render time)

Reviewed By: josephsavona

Differential Revision: D23171834

fbshipit-source-id: e368f913150ea1aecac92cd66c0d7083635671a2

Author: Juan Tejada

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

@@ -69,6 +69,8 @@ const ID = '12345';
 const variables = {id: '4'};
 
 let callLoadQuery;
+let environment;
+let fetch;
 let writeDataToStore;
 let sink;
 let next;
@@ -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
@@ -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);
@@ -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);

packages/relay-experimental/__tests__/loadQuery-store-behavior-test.js

@@ -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);

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

@@ -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) => {

packages/relay-experimental/loadQuery.js

@@ -21,6 +21,7 @@ const {
   createOperationDescriptor,
   getRequest,
   Observable,
+  __internal: {fetchQueryDeduped},
 } = require('relay-runtime');
 
 import type {
@@ -30,6 +31,7 @@ import type {
 } from './EntryPointTypes.flow';
 import type {
   IEnvironment,
+  OperationDescriptor,
   OperationType,
   GraphQLTaggedNode,
   GraphQLResponse,
@@ -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
@@ -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
@@ -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;
@@ -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 => {
@@ -175,7 +228,9 @@ function loadQuery<TQuery: OperationType, TEnvironmentProviderOptions>(
             preloadedModule,
             variables,
           );
-          executeWithSource(operation, source);
+          executeDeduped(operation, () =>
+            executeWithNetworkSource(operation, networkObservable),
+          );
         },
       ));
       if (!environment.isServer()) {
@@ -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);
   }
 
@@ -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);

packages/relay-experimental/usePreloadedQuery.js

@@ -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';
@@ -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,

packages/relay-test-utils/RelayModernMockEnvironment.js

@@ -480,6 +480,7 @@ function createMockEnvironment(
     mockDisposableMethod(environment, 'subscribe');
     mockDisposableMethod(environment, 'retain');
     mockObservableMethod(environment, 'execute');
+    mockObservableMethod(environment, 'executeWithSource');
     mockObservableMethod(environment, 'executeMutation');
 
     mockInstanceMethod(store, 'getSource');