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

Weird query lifecycle transitions #38

Closed
olistic opened this issue May 30, 2019 · 9 comments · Fixed by #42
Closed

Weird query lifecycle transitions #38

olistic opened this issue May 30, 2019 · 9 comments · Fixed by #42

Comments

@olistic
Copy link

olistic commented May 30, 2019

Hey there! I discovered your library yesterday and decided to give it a try because I think its caching mechanism is superior to what graphql-hooks (what I'm currently using) offers.

What I'm doing

To make migration easier, I'm creating the following hook:

import { useGraphQL } from 'graphql-react';

const fetchOptionsOverride = options => {
  options.url = '/api/graphql';
};

const useQuery = (query, { variables } = {}) => {
  const {
    loading,
    cacheValue: { data, fetchError, httpError, parseError, graphQLErrors } = {},
  } = useGraphQL({
    fetchOptionsOverride,
    operation: {
      variables,
      query,
    },
  });

  const error = fetchError || httpError || parseError || graphQLErrors;

  console.log({ loading, error, data }); // <- Remember this call to console.log.

  return {
    loading,
    error,
    data,
  };
};

export default useQuery;

What I expect

Later, in my component I expect to be able to use useQuery like this:

const postQuery = /* GraphQL */ `...`;

const Post = ({ postId }) => {
  const { loading, error, data } = useQuery(postQuery, {
    variables: {
      postId,
    },
  });

  if (loading) {
    return 'Loading...';
  }

  if (error) {
    return 'Something went wrong.';
  }

  return (
    <article>
      <h1>{data.post.title}</h1>
      <p>{data.post.content}</p>
    </article>
  );
};

What actually happens

Blows up when trying to access data.post.title. I traced that down to the different values loading, error and data have during the query lifecycle:

  1. { loading: false, error: undefined, data: undefined }

All three values are falsy at the beginning, thus provoking the issue I just mentioned with my component's code.

  1. { loading: true, error: undefined, data: undefined }

loading is set to true when load() is called inside useGraphQL.

  1. { loading: false, error: undefined, data: undefined }

When that finishes, loading returns to false, but neither error nor data are set (problematic, if you ask me).

  1. { loading: false, error: undefined, data: {…} }

Finally, data is populated with the result of the GraphQL query.

I would expect this sequence to be like this instead:

  1. { loading: true, error: undefined, data: undefined }
  2. { loading: false, error: undefined, data: {…} }

Am I doing something wrong in my useQuery hook? Is this the expected behavior?

@jaydenseric
Copy link
Owner

I'm not sure why at first you're getting loading: false, since in the tests that does not happen:

t.equals(loading, true, 'Hook return `loading`')

@olistic
Copy link
Author

olistic commented Jun 2, 2019

Yeah, weird. Let me try to reproduce the issue in a codesandbox. If I'm doing something wrong I'll probably find out then. I'll keep you posted.

@olistic
Copy link
Author

olistic commented Jun 2, 2019

@jaydenseric I was able to reproduce it in this codesandbox.

@jaydenseric
Copy link
Owner

jaydenseric commented Jun 5, 2019

Bear with me, but I think this may be working as intended.

It's been a while since I dove deep into React hooks, exploring all the gotchas (console.log debugging hooks is confusing), but here are my rusty thoughts…

The first log of loading: false is correct; because that state only updates once it actually starts loading, which is not until after it mounts. The first console.log can be from a render that happened before the useEffect triggers loading. Note that in the situation where the exact same query is already loading in the client (triggered by something somewhere else) at the time the component mounts, the first log may be loading: true.

Now my memory may be hazy/wrong, but when multiple useState values are updated internally there can be multiple re-renders, but you don't actually see each re-render happen in the real DOM if they happen quickly.

You do need to make sure you nest any JSX using cacheValue.data in a conditional that checks data actually exists first.

@jaydenseric
Copy link
Owner

Closing because I'm pretty sure everything is working as intended; the general rule is to account for cacheValue sometimes not being defined, as documented in all the useGraphQL examples:

const { loading, cacheValue = {} } = useGraphQL({
  // …
})

https://github.com/jaydenseric/graphql-react/tree/v9.1.0#examples-8

I always do cacheValue = {}, and haven't had any problems in all my apps.

It's potentially useful to be able to tell when rendering if there is no cache for a particular query, so it's probably good to leave the API the way it is and not default cacheValue to an empty object.

@Grsmto
Copy link
Contributor

Grsmto commented Jan 27, 2020

I'm having the same issue and I did a reproductible example with the doc snippet fetching a Pokemon: https://codesandbox.io/s/gracious-leakey-hisdk

As described by @olistic there is a state that imo should not happen:

  1. loading starts: { loading: true, data: undefined }

  2. loading is finished but data is still undefined?? { loading: false, data: undefined }
    This state does not make sense because it then trigger the if condition displaying "Error!" in the CodeSandbox (it's not visible on the example since it's instantly removed).

  3. One tick later, finally the data is there: { loading: false, data: {…} }

As you can see in that CodeSandbox I don't think there is any external hook hijacking the render loop in the middle that could cause that. So it seems to come from graphql-react.
In my app this is actually causing an issue since I'm displaying an error message with a transition animation so even if the wrong state is really short, it stills trigger the in/out animation creating a flash of the error state.

@jaydenseric
Copy link
Owner

@Grsmto are you using console.log to debug the hook return value? A quick guess is that internally two useState values are being set, one immediately after the other, which in React can result in two very fast "renders", but only one change to the DOM.

I plan to look into how these lifecycle gotchas can be improved when I start work on the next version; I've been working hard on other projects lately (several new testing related packages, and graphql-upload v10), and now I've got full time contract work to contend with.

@Grsmto
Copy link
Contributor

Grsmto commented Jan 28, 2020

Thanks for your quick reply.

internally two useState values are being set, one immediately after the other, which in React can result in two very fast "renders"

Actually as you can see in this codesandbox example, that's not the case: https://codesandbox.io/s/green-architecture-023dc
Doing 2 following setState still will trigger 1 single render with both setState applied at the same time (not one after the other).

Anyway I will try to have a look at the source code of the useGraphQL see if I find something. Good luck with your new contract!

@jaydenseric jaydenseric reopened this Jan 28, 2020
jaydenseric added a commit that referenced this issue Mar 25, 2020
Use ReactDOM.unstable_batchedUpdates in the useGraphQL React hook to reduce the number of renders when loading completes, fixing #38 .
@jaydenseric
Copy link
Owner

This should be improved in v10.0.0 🚀

denisp22 added a commit to denisp22/graphql-react that referenced this issue Jul 27, 2023
Use ReactDOM.unstable_batchedUpdates in the useGraphQL React hook to reduce the number of renders when loading completes, fixing jaydenseric/graphql-react#38 .
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

Successfully merging a pull request may close this issue.

3 participants