From d513e42f881eccab71c8ed5448a66fed13502be2 Mon Sep 17 00:00:00 2001 From: Andrey Lunyov Date: Thu, 18 Jan 2024 12:23:00 -0800 Subject: [PATCH] Suspend on active promises for client edge queries in the new hooks implementation Reviewed By: captbaritone Differential Revision: D52837890 fbshipit-source-id: 5197a2d96f2b46f4f9ee66f52112c238dc1a197d --- .../react-relay/__tests__/ClientEdges-test.js | 474 +++++++++--------- .../useFragmentInternal_EXPERIMENTAL.js | 38 +- 2 files changed, 270 insertions(+), 242 deletions(-) diff --git a/packages/react-relay/__tests__/ClientEdges-test.js b/packages/react-relay/__tests__/ClientEdges-test.js index bf5f78c21bc90..8ba3e02967e32 100644 --- a/packages/react-relay/__tests__/ClientEdges-test.js +++ b/packages/react-relay/__tests__/ClientEdges-test.js @@ -11,6 +11,7 @@ 'use strict'; +const useFragmentInternal_EXPERIMENTAL = require('../relay-hooks/experimental/useFragmentInternal_EXPERIMENTAL'); const React = require('react'); const {RelayEnvironmentProvider, useLazyLoadQuery} = require('react-relay'); const TestRenderer = require('react-test-renderer'); @@ -41,271 +42,286 @@ afterEach(() => { RelayFeatureFlags.ENABLE_CLIENT_EDGES = false; }); -describe('ClientEdges', () => { - let networkSink; - let environment; - let fetchFn; - beforeEach(() => { - fetchFn = jest.fn(() => - // $FlowFixMe[missing-local-annot] Error found while enabling LTI on this file - RelayObservable.create(sink => { - networkSink = sink; - }), - ); +describe.each(['Legacy', 'Experimental'])( + 'Client Edges (Hook implementation: %s)', + (implementation: 'Legacy' | 'Experimental') => { + let networkSink; + let environment; + let fetchFn; + beforeEach(() => { + if (implementation === 'Experimental') { + jest.mock('../relay-hooks/HooksImplementation', () => { + return { + get() { + return { + useFragment__internal: useFragmentInternal_EXPERIMENTAL, + }; + }, + }; + }); + } - environment = new Environment({ - store: new LiveResolverStore( - new RecordSource({ - 'client:root': { - __id: 'client:root', - __typename: '__Root', - me: {__ref: '1'}, - }, - '1': { - __id: '1', - id: '1', - __typename: 'User', - }, + fetchFn = jest.fn(() => + // $FlowFixMe[missing-local-annot] Error found while enabling LTI on this file + RelayObservable.create(sink => { + networkSink = sink; }), - ), - // $FlowFixMe[invalid-tuple-arity] Error found while enabling LTI on this file - network: Network.create(fetchFn), + ); + + environment = new Environment({ + store: new LiveResolverStore( + new RecordSource({ + 'client:root': { + __id: 'client:root', + __typename: '__Root', + me: {__ref: '1'}, + }, + '1': { + __id: '1', + id: '1', + __typename: 'User', + }, + }), + ), + // $FlowFixMe[invalid-tuple-arity] Error found while enabling LTI on this file + network: Network.create(fetchFn), + }); }); - }); - it('should fetch and render client-edge query', () => { - function TestComponent() { - return ( - - - - - - ); - } + it('should fetch and render client-edge query', () => { + function TestComponent() { + return ( + + + + + + ); + } - const variables = {id: '4'}; - function InnerComponent() { - const data = useLazyLoadQuery( - graphql` - query ClientEdgesTest1Query($id: ID!) { - me { - client_node(id: $id) @waterfall { - ... on User { - name + const variables = {id: '4'}; + function InnerComponent() { + const data = useLazyLoadQuery( + graphql` + query ClientEdgesTest1Query($id: ID!) { + me { + client_node(id: $id) @waterfall { + ... on User { + name + } } } } - } - `, - variables, - ); - return data.me?.client_node?.name; - } + `, + variables, + ); + return data.me?.client_node?.name; + } - let renderer; - TestRenderer.act(() => { - renderer = TestRenderer.create(); - }); - expect(fetchFn.mock.calls.length).toEqual(1); - // We should send the client-edge query - // $FlowFixMe[invalid-tuple-index] Error found while enabling LTI on this file - expect(fetchFn.mock.calls[0][0].name).toBe( - 'ClientEdgeQuery_ClientEdgesTest1Query_me__client_node', - ); - // Check variables - // $FlowFixMe[invalid-tuple-index] Error found while enabling LTI on this file - expect(fetchFn.mock.calls[0][1]).toEqual(variables); - expect(renderer?.toJSON()).toBe('Loading'); + let renderer; + TestRenderer.act(() => { + renderer = TestRenderer.create(); + }); + expect(fetchFn.mock.calls.length).toEqual(1); + // We should send the client-edge query + // $FlowFixMe[invalid-tuple-index] Error found while enabling LTI on this file + expect(fetchFn.mock.calls[0][0].name).toBe( + 'ClientEdgeQuery_ClientEdgesTest1Query_me__client_node', + ); + // Check variables + // $FlowFixMe[invalid-tuple-index] Error found while enabling LTI on this file + expect(fetchFn.mock.calls[0][1]).toEqual(variables); + expect(renderer?.toJSON()).toBe('Loading'); - TestRenderer.act(() => { - // This should resolve client-edge query - networkSink.next({ - data: { - node: { - id: '4', - __typename: 'User', - name: 'Alice', + TestRenderer.act(() => { + // This should resolve client-edge query + networkSink.next({ + data: { + node: { + id: '4', + __typename: 'User', + name: 'Alice', + }, }, - }, + }); + jest.runAllImmediates(); }); - jest.runAllImmediates(); + expect(renderer?.toJSON()).toBe('Alice'); }); - expect(renderer?.toJSON()).toBe('Alice'); - }); - // The Relay store does not have a concept of _records_ being null. This means that when a Node - // query returns null, we can't actally write to the store "The record with this ID is null". - // Instead, we just write that `node(id: 4): null` into the root record in the store. - // - // This is a general limitiaton of node fetches in Relay today. - it('should fetch and render `undefined` for client-edge to server query that returns `null`.', () => { - function TestComponent() { - return ( - - - - - - ); - } + // The Relay store does not have a concept of _records_ being null. This means that when a Node + // query returns null, we can't actally write to the store "The record with this ID is null". + // Instead, we just write that `node(id: 4): null` into the root record in the store. + // + // This is a general limitiaton of node fetches in Relay today. + it('should fetch and render `undefined` for client-edge to server query that returns `null`.', () => { + function TestComponent() { + return ( + + + + + + ); + } - const variables = {id: '4'}; - function InnerComponent() { - const data = useLazyLoadQuery( - graphql` - query ClientEdgesTest2Query($id: ID!) { - me { - client_node(id: $id) @waterfall { - ... on User { - name + const variables = {id: '4'}; + function InnerComponent() { + const data = useLazyLoadQuery( + graphql` + query ClientEdgesTest2Query($id: ID!) { + me { + client_node(id: $id) @waterfall { + ... on User { + name + } } } } - } - `, - variables, - ); - if (data.me?.client_node === undefined) { - return 'client_node is undefined'; + `, + variables, + ); + if (data.me?.client_node === undefined) { + return 'client_node is undefined'; + } + return data.me?.client_node?.name ?? 'MISSING'; } - return data.me?.client_node?.name ?? 'MISSING'; - } - let renderer; - TestRenderer.act(() => { - renderer = TestRenderer.create(); - }); - expect(fetchFn.mock.calls.length).toEqual(1); - // We should send the client-edge query - // $FlowFixMe[invalid-tuple-index] Error found while enabling LTI on this file - expect(fetchFn.mock.calls[0][0].name).toBe( - 'ClientEdgeQuery_ClientEdgesTest2Query_me__client_node', - ); - // Check variables - // $FlowFixMe[invalid-tuple-index] Error found while enabling LTI on this file - expect(fetchFn.mock.calls[0][1]).toEqual(variables); - expect(renderer?.toJSON()).toBe('Loading'); + let renderer; + TestRenderer.act(() => { + renderer = TestRenderer.create(); + }); + expect(fetchFn.mock.calls.length).toEqual(1); + // We should send the client-edge query + // $FlowFixMe[invalid-tuple-index] Error found while enabling LTI on this file + expect(fetchFn.mock.calls[0][0].name).toBe( + 'ClientEdgeQuery_ClientEdgesTest2Query_me__client_node', + ); + // Check variables + // $FlowFixMe[invalid-tuple-index] Error found while enabling LTI on this file + expect(fetchFn.mock.calls[0][1]).toEqual(variables); + expect(renderer?.toJSON()).toBe('Loading'); - TestRenderer.act(() => { - // This should resolve client-edge query - networkSink.next({ - data: { - node: null, - }, + TestRenderer.act(() => { + // This should resolve client-edge query + networkSink.next({ + data: { + node: null, + }, + }); + // It is important to complete network request here, + // otherwise, client-edge query will think that the query is still in progress + // and will show a suspense placeholder + networkSink.complete(); + jest.runAllImmediates(); }); - // It is important to complete network request here, - // otherwise, client-edge query will think that the query is still in progress - // and will show a suspense placeholder - networkSink.complete(); - jest.runAllImmediates(); + expect(renderer?.toJSON()).toBe('client_node is undefined'); + expect(fetchFn.mock.calls.length).toBe(1); }); - expect(renderer?.toJSON()).toBe('client_node is undefined'); - expect(fetchFn.mock.calls.length).toBe(1); - }); - it('should throw for missing client-edge field data marked with @required', () => { - function TestComponent() { - return ( - - - - - - ); - } + it('should throw for missing client-edge field data marked with @required', () => { + function TestComponent() { + return ( + + + + + + ); + } - const variables = {id: '4'}; - function InnerComponent() { - const data = useLazyLoadQuery( - graphql` - query ClientEdgesTest3Query($id: ID!) { - me { - client_node(id: $id) @waterfall @required(action: THROW) { - ... on User { - name + const variables = {id: '4'}; + function InnerComponent() { + const data = useLazyLoadQuery( + graphql` + query ClientEdgesTest3Query($id: ID!) { + me { + client_node(id: $id) @waterfall @required(action: THROW) { + ... on User { + name + } } } } - } - `, - variables, - ); - return data.me?.client_node?.name; - } - - let renderer; - TestRenderer.act(() => { - renderer = TestRenderer.create(); - }); - expect(fetchFn.mock.calls.length).toEqual(1); - // We should send the client-edge query - // $FlowFixMe[invalid-tuple-index] Error found while enabling LTI on this file - expect(fetchFn.mock.calls[0][0].name).toBe( - 'ClientEdgeQuery_ClientEdgesTest3Query_me__client_node', - ); - // Check variables - // $FlowFixMe[invalid-tuple-index] Error found while enabling LTI on this file - expect(fetchFn.mock.calls[0][1]).toEqual(variables); - expect(renderer?.toJSON()).toBe('Loading'); + `, + variables, + ); + return data.me?.client_node?.name; + } - TestRenderer.act(() => { - networkSink.next({ - data: { - node: null, - }, + let renderer; + TestRenderer.act(() => { + renderer = TestRenderer.create(); }); - jest.runAllImmediates(); - }); - // Still waiting, maybe the data will be there - expect(renderer?.toJSON()).toBe('Loading'); + expect(fetchFn.mock.calls.length).toEqual(1); + // We should send the client-edge query + // $FlowFixMe[invalid-tuple-index] Error found while enabling LTI on this file + expect(fetchFn.mock.calls[0][0].name).toBe( + 'ClientEdgeQuery_ClientEdgesTest3Query_me__client_node', + ); + // Check variables + // $FlowFixMe[invalid-tuple-index] Error found while enabling LTI on this file + expect(fetchFn.mock.calls[0][1]).toEqual(variables); + expect(renderer?.toJSON()).toBe('Loading'); - expect(() => { TestRenderer.act(() => { - // This should resolve client-edge query - networkSink.complete(); + networkSink.next({ + data: { + node: null, + }, + }); jest.runAllImmediates(); }); - }).toThrow( - "Relay: Missing @required value at path 'me.client_node' in 'ClientEdgesTest3Query'.", - ); - expect(renderer?.toJSON()).toBe(null); - }); + // Still waiting, maybe the data will be there + expect(renderer?.toJSON()).toBe('Loading'); - it('should throw for missing client-edge (client object) field data marked with @required', () => { - function TestComponent() { - return ( - - - - - + expect(() => { + TestRenderer.act(() => { + // This should resolve client-edge query + networkSink.complete(); + jest.runAllImmediates(); + }); + }).toThrow( + "Relay: Missing @required value at path 'me.client_node' in 'ClientEdgesTest3Query'.", ); - } - // See UserClientEdgeClientObjectResolver: for `0` we should return `null` for `client_object`. - const variables = {id: '0'}; - function InnerComponent() { - const data = useLazyLoadQuery( - graphql` - query ClientEdgesTest4Query($id: ID!) { - me { - client_object(id: $id) @required(action: THROW) { - description + expect(renderer?.toJSON()).toBe(null); + }); + + it('should throw for missing client-edge (client object) field data marked with @required', () => { + function TestComponent() { + return ( + + + + + + ); + } + // See UserClientEdgeClientObjectResolver: for `0` we should return `null` for `client_object`. + const variables = {id: '0'}; + function InnerComponent() { + const data = useLazyLoadQuery( + graphql` + query ClientEdgesTest4Query($id: ID!) { + me { + client_object(id: $id) @required(action: THROW) { + description + } } } - } - `, - variables, + `, + variables, + ); + return data.me?.client_object?.description; + } + expect(() => { + TestRenderer.act(() => { + TestRenderer.create(); + }); + }).toThrow( + "Relay: Missing @required value at path 'me.client_object' in 'ClientEdgesTest4Query'.", ); - return data.me?.client_object?.description; - } - expect(() => { - TestRenderer.act(() => { - TestRenderer.create(); - }); - }).toThrow( - "Relay: Missing @required value at path 'me.client_object' in 'ClientEdgesTest4Query'.", - ); - expect(fetchFn.mock.calls.length).toEqual(0); - }); -}); + expect(fetchFn.mock.calls.length).toEqual(0); + }); + }, +); diff --git a/packages/react-relay/relay-hooks/experimental/useFragmentInternal_EXPERIMENTAL.js b/packages/react-relay/relay-hooks/experimental/useFragmentInternal_EXPERIMENTAL.js index 42549144f6c9e..0c326bef81290 100644 --- a/packages/react-relay/relay-hooks/experimental/useFragmentInternal_EXPERIMENTAL.js +++ b/packages/react-relay/relay-hooks/experimental/useFragmentInternal_EXPERIMENTAL.js @@ -31,7 +31,7 @@ const useRelayEnvironment = require('../useRelayEnvironment'); const invariant = require('invariant'); const {useDebugValue, useEffect, useMemo, useRef, useState} = require('react'); const { - __internal: {fetchQuery: fetchQueryInternal}, + __internal: {fetchQuery: fetchQueryInternal, getPromiseForActiveRequest}, RelayFeatureFlags, areEqualSelectors, createOperationDescriptor, @@ -218,7 +218,7 @@ function handleMissingClientEdge( parentFragmentRef: mixed, missingClientEdgeRequestInfo: MissingClientEdgeRequestInfo, queryOptions?: FragmentQueryOptions, -): QueryResult { +): [QueryResult, ?Promise] { const originalVariables = getVariablesFromFragment( parentFragmentNode, parentFragmentRef, @@ -237,11 +237,16 @@ function handleMissingClientEdge( // according to the component mount/suspense cycle; QueryResource // already handles this by itself. const QueryResource = getQueryResourceForEnvironment(environment); - return QueryResource.prepare( + const queryResult = QueryResource.prepare( queryOperationDescriptor, fetchQueryInternal(environment, queryOperationDescriptor), queryOptions?.fetchPolicy, ); + + return [ + queryResult, + getPromiseForActiveRequest(environment, queryOperationDescriptor.request), + ]; } function subscribeToSnapshot( @@ -461,27 +466,34 @@ function useFragmentInternal_EXPERIMENTAL( // a static (constant) property of the fragment. In practice, this effect will // always or never run for a given invocation of this hook. // eslint-disable-next-line react-hooks/rules-of-hooks - const clientEdgeQueries = useMemo(() => { + const [clientEdgeQueries, activeRequestPromises] = useMemo(() => { const missingClientEdges = getMissingClientEdges(state); // eslint-disable-next-line no-shadow let clientEdgeQueries; + const activeRequestPromises = []; if (missingClientEdges?.length) { clientEdgeQueries = ([]: Array); for (const edge of missingClientEdges) { - clientEdgeQueries.push( - handleMissingClientEdge( - environment, - fragmentNode, - fragmentRef, - edge, - queryOptions, - ), + const [queryResult, requestPromise] = handleMissingClientEdge( + environment, + fragmentNode, + fragmentRef, + edge, + queryOptions, ); + clientEdgeQueries.push(queryResult); + if (requestPromise != null) { + activeRequestPromises.push(requestPromise); + } } } - return clientEdgeQueries; + return [clientEdgeQueries, activeRequestPromises]; }, [state, environment, fragmentNode, fragmentRef, queryOptions]); + if (activeRequestPromises.length) { + throw Promise.all(activeRequestPromises); + } + // See above note // eslint-disable-next-line react-hooks/rules-of-hooks useEffect(() => {