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

Relay: loadQuery should not be called inside a React render function. #3449

Open
simkessy opened this issue Apr 7, 2021 · 13 comments
Open

Comments

@simkessy
Copy link

simkessy commented Apr 7, 2021

I created a RelayTestComponent for re-use during testing

import { Suspense } from 'react';
import { loadQuery, RelayEnvironmentProvider } from 'react-relay/hooks';
import { createMockEnvironment, MockPayloadGenerator } from 'relay-test-utils';

import { Router } from 'react-router-dom';
import { createMemoryHistory } from 'history';

export default ({ resolver, query, variables = {}, fetchPolicy, children }) => {
  // Necessary for router provider
  const history = createMemoryHistory();

  // Mock relay environment
  const mockEnvironment = createMockEnvironment();

  mockEnvironment.mock.queueOperationResolver(operation =>
    // Resolve = data you want returned
    MockPayloadGenerator.generate(operation, resolver),
  );

  // Execute Query
  mockEnvironment.mock.queuePendingOperation(query, variables);

  const preloadedQuery = loadQuery(mockEnvironment, query, variables, {
    fetchPolicy: 'store-or-network',
    ...fetchPolicy,
  });

  return (
    <RelayEnvironmentProvider environment={mockEnvironment}>
            <Router history={history}>
              <Suspense fallback="Loading...">{children(preloadedQuery)}</Suspense>
            </Router>
    </RelayEnvironmentProvider>
  );
};

Then I have a component which uses preloadedQuery and renders the component im trying to test:

const Component = ({ preloadedQuery }) => {
  const data = usePreloadedQuery(BANNER_TEST_QUERY, preloadedQuery);

  /* COMPONENT HERE */
  return <Banner viewer={data.viewer} />;
};

Im my test I do:

render(
  <RelayTestComponent resolver={resolver || defaultResolver} query={BannerTestQuery}>
    {preloadedQuery => <Component preloadedQuery={preloadedQuery} />}
  </RelayTestComponent>
)

But now im getting this error for every test after the 1st test:

  Warning: Relay: `loadQuery` should not be called inside a React render function.

      24 |   mockEnvironment.mock.queuePendingOperation(query, variables);
      25 |
    > 26 |   const preloadedQuery = loadQuery(mockEnvironment, query, variables, {
         |                          ^
      27 |     fetchPolicy: 'store-or-network',
      28 |     ...fetchPolicy,
      29 |   });
@phtmgt
Copy link

phtmgt commented May 31, 2021

Same issue here. This seems extremely counterintuitive: How do I pass the variables to loadQuery, if it's not called in the functional component? To load a specific entity, I need its ID, which I get from either context or props and these are only accessible within the functional component.

Am I missing something huge here? V11 is a huge departure from V10, I hope this is for the better long-term, as it is causing us lots of headaches with understanding how everything functions now.

@josephsavona
Copy link
Contributor

josephsavona commented Jun 1, 2021

Note that loadQuery() is not designed to be called during render, and doing so will cause problems in upcoming versions of React. We recommend calling loadQuery() prior to calling render() - in your example, this would be something like:

const preloadedQuery = loadQuery(....);
render(
  <RelayTestComponent environment={...}>
    <Component preloadedQuery={preloadedQuery} />
  </RelayTestComponent>
)

How do I pass the variables to loadQuery, if it's not called in the functional component? To load a specific entity, I need its ID, which I get from either context or props and these are only accessible within the functional component.

Note that loadQuery() + usePreloadedQuery() generally requires more setup to use. The easiest way to get started fetching data is with useLazyLoadQuery().

@phtmgt
Copy link

phtmgt commented Jun 2, 2021

I was able to finally loosely grasp how everything is supposed to work by looking at the migration section here:

https://relay.dev/docs/migration-and-compatibility/relay-hooks-and-legacy-container-apis/

The hooks documentation is very confusing for someone like me coming from the Legacy APIs.

@jamietdavidson
Copy link

Just ran into this issue, and now I have to rethink everything.

To load a specific entity, I need its ID, which I get from either context or props and these are only accessible within the functional component.

This was my exact thinking.

How have others approached this problem?

@phtmgt
Copy link

phtmgt commented Jun 5, 2021

@josephsavona
Copy link
Contributor

@jamietdavidson That's a reasonable use-case for useLazyLoadQuery()

@jamietdavidson
Copy link

jamietdavidson commented Jun 8, 2021

@plamenh I went through that example thoroughly, and encountered the same error when calling the loadQuery function that is returned from the useQueryLoader hook. Although strangely enough, I am not calling it inside of a render function.

My component has been simplified down to this:

import { Card, CardContent, Grid } from '@material-ui/core';
import { experimentalStyled as styled } from '@material-ui/core/styles';
import { TreeView } from '@material-ui/lab';
import React from 'react';
import { graphql, useQueryLoader } from 'react-relay';

const TreeViewStyle = styled(TreeView)(({ theme }) => ({
  height: 240,
  flexGrow: 1,
  maxWidth: 400
}));

type GraphEditorProps = { };

function GraphEditor({ }: GraphEditorProps) {
  const query = graphql`
    query GraphEditorQuery($uid: UUID!) {
      genericGraphObject(uid: $uid) {
        ok
        data {
          id
          fields {
            key
            value
          }
          edges {
            key
            uid
          }
          edgeCollections {
            key
            uids
          }
        }
      }
    }
  `;
  const [queryRef, loadQuery] = useQueryLoader(query);
  const root = loadQuery({ uid: '09bad3ff-f3dd-4abf-95d0-461fc44231de' });
  console.log('ROOT', root);

  return (
    <Card>
      <CardContent>
        <Grid item xs={12} md={4}>
          Debugging
        </Grid>
      </CardContent>
    </Card>
  );
}

export default GraphEditor;

And I am receiving these console messages in return:
Screen Shot 2021-06-07 at 5 25 02 PM

Eventually, I get a too many re-renders error.

I am also receiving the same too many re-renders error when I use useLazyLoadQuery (calling it in the same spot as above).

Note: I have confirmed that the API call is being made and that the data is being returned on the backend successfully, and taking out the API call renders my page correctly, displaying my "Debugging" text.

What am I missing?

@jamietdavidson
Copy link

jamietdavidson commented Jun 8, 2021

Got it. It needed to be wrapped in Suspense.

While this is completely my bad, I have to say that I found this article was alot easier to follow than the Relay docs.

I also had to use an useEffect to get rid of the error. While the relay docs had a similiar example where they used useCallback, it was only on the migration guide that @plamenh posted, and not on the hook docs itself. This was a large source of confusion for me.

In any case, I hope this offers some help to anyone else that stumbles into this problem as a beginner to React (like myself).

The useEffect I implemented looked something like this:

  // Load root graph data
  React.useEffect(() => {
    const queryRef = loadQuery(environment, query, { uid: rootQueryId });
    setRootQueryRef(queryRef);
  }, [rootQueryId]);

Best,
James

@josephsavona
Copy link
Contributor

josephsavona commented Jun 9, 2021

@jamietdavidson Note that the example usage of usePreloadedQuery in that article is an anti-pattern. Instead of calling useQueryLoader(), invoking loadQuery() in a useEfect, and then rendering with usePreloadedQuery(), we recommend calling just useLazyLoadQuery(). The latter is both easier and more efficient, as it will start fetching data earlier.

Stated conversely, we only recommend usePreloadedQuery() when you're able to start loading the query in advance, for example via router integration.

@stale
Copy link

stale bot commented Jan 9, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jan 9, 2022
@alex-statsig
Copy link
Contributor

alex-statsig commented Jan 26, 2022

@jamietdavidson Note that the example usage of usePreloadedQuery in that article is an anti-pattern. Instead of calling useQueryLoader(), invoking loadQuery() in a useEfect, and then rendering with usePreloadedQuery(), we recommend calling just useLazyLoadQuery(). The latter is both easier and more efficient, as it will start fetching data earlier.

Stated conversely, we only recommend usePreloadedQuery() when you're able to start loading the query in advance, for example via router integration.

@josephsavona Suppose a component has two queries to fire. In that case, calling useLazyLoadQuery twice will result in a waterfall. What's the recommendation in this case? Obviously preloading both queries is ideal, but working with nextjs its not quite feasible yet. I thought a loadQuery in useRef/useMemo might be a good compromise, but it would trigger loadQuery during render.

@stale stale bot removed the wontfix label Jan 26, 2022
@maraisr
Copy link
Contributor

maraisr commented Jan 31, 2022

recently ran into a similar issue with Storybook and decorators. FWIW — useMemo(() => ...) seems to squash that relay warning.

@emwalker
Copy link

emwalker commented Sep 18, 2022

A point of confusion can arise in connection with the docs for useLazyLoadQuery, which say:

Hook used to fetch a GraphQL query during render. This hook can trigger multiple nested or waterfalling round trips if used without caution, and waits until render to start a data fetch (when it can usually start a lot sooner than render), thereby degrading performance. Instead, prefer usePreloadedQuery.

This makes it sound like usePreloadedQuery is to be preferred, and useLazyLoadQuery should be used with caution in corner cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants