apollographql / react-apollo

:recycle: React integration for Apollo Client
https://www.apollographql.com/docs/react/
MIT License
6.85k stars 787 forks source link

useLazyQuery onCompleted called on every re-render #3505

Closed PovilasSlekys closed 5 years ago

PovilasSlekys commented 5 years ago

Intended outcome: useLazyQuery only calls onCompleted on a successful network request

Actual outcome: useLazyQuery calls onCompleted after every re-render even if the result is being taken from the cache

Version

npmPackages: apollo-cache-inmemory: ^1.6.3 => 1.6.3 apollo-client: ^2.6.4 => 2.6.4 apollo-link-context: ^1.0.19 => 1.0.19 apollo-link-http: ^1.5.16 => 1.5.16 react-apollo: ^3.1.1 => 3.1.1

huongdevvn commented 5 years ago

The same happen for me. onCompleted of useLazyQuery is called only once when I execute simple functions like console.log

const [fetchLazy] = useLazyQuery(LAZY_QUERY, {
    variables: { id },
    onCompleted: data => {
      console.log('data ', data);
    }
  });

But if I execute a function returned by React hooks like: useState or useReducer, onCompleted will happen many times or infinite times.

  const [data, setData] = useState(null);

  const [fetchLazy] = useLazyQuery(LAZY_QUERY, {
    variables: { id },
    onCompleted: data => {
      console.log('data ', data);
      setData(data); // multiple times
    }
  });
  const [state, dispatch] = useReducer(reducer, initialState);

  const [fetchLazy] = useLazyQuery(LAZY_QUERY, {
    variables: { id },
    onCompleted: data => {
      console.log('data ', data);
      dispatch({ type: 'completed', payload: 'data' }); // infinite times
    }
  });

Version

"@apollo/react-hooks": "^3.1.1",
"@apollo/react-ssr": "^3.1.1",
"apollo-cache-inmemory": "^1.6.3",
"apollo-client": "^2.6.4",
"apollo-link-context": "^1.0.19",
"apollo-link-error": "^1.1.12",
"apollo-link-http": "^1.5.16",
huongdevvn commented 5 years ago

@hwillson Could you take a look?

vilvaathibanpb commented 5 years ago

I can work on this issue if @hwillson or @bsonntag haven't started working on this.

bsonntag commented 5 years ago

@vilvaathibanpb I took a look and figured the problem is here: https://github.com/apollographql/react-apollo/blob/master/packages/hooks/src/utils/useBaseQuery.ts#L57. I didn't start working on fix tho, so go ahead.

vilvaathibanpb commented 5 years ago

@bsonntag Sure, Let me raise a PR against it. Thanks

vilvaathibanpb commented 5 years ago

Hey @bsonntag , I was checking the issue and my observations are:

  1. For a normal query, the useEffect hook is checked only once for any state change.
  2. For a lazy query, the useEffect is checked for multiple times.

Do you still think, we should work on fixing the undefinded here ? https://github.com/apollographql/react-apollo/blob/master/packages/hooks/src/utils/useBaseQuery.ts#L57 .

I sense that, the useEffect hook shouldn't been checked when there is a state change? What do you think?

bsonntag commented 5 years ago

My understanding is that useLazyQuery's onCompleted callback should only be called when a call to the query completes.

Right now onCompleted is being called on each render after the first completed call, so if the component re-renders for any other reason (like a setState or a dispatch) onCompleted will be called again.

For example, the following component will enter an infinite re-render loop when the query completes:


function Users() {
  const [count, setCount] = useState(0);
  const [fetchUsers, { data, loading }] = useLazyQuery(usersQuery, {
    onCompleted: () => setCount(count => count + 1)
  });

  return (
    <>
      <button onClick={() => fetchUsers()}>Fetch</button>
      {/* Render users */}
    </>
  );
}
vilvaathibanpb commented 5 years ago

@bsonntag Yes. I completely get the issue. But when I debugged the issue, I dont think it is enough if we just change the undefinded condition when lazy is true, under useEffect here - https://github.com/apollographql/react-apollo/blob/master/packages/hooks/src/utils/useBaseQuery.ts#L57 .

May be this useEffect is reason for queryData.afterExecute which executes the onCompleted method. But, what I couldnt understand is, why useEffect is been checked for state change. In a normal query, the useEffect is checked / executed only when the query is fired. But for lazy query, it gets checked for every state change. Do you have any idea on it? Else I could dig deeper.

bsonntag commented 5 years ago

All I know is that that change was introduced on https://github.com/apollographql/react-apollo/pull/3497

Maybe @hwillson can explain?

vilvaathibanpb commented 5 years ago

Sure. lets wait for him to respond

dnknitro commented 5 years ago

Any update on this issue?

vilvaathibanpb commented 5 years ago

Hi @dnknitro, I am waiting for @hwillson to respond. If he doesn't by Tommorow, I will dig deep into it myself

Siggnja commented 5 years ago

Any updates?

holliepearson commented 5 years ago

Any progress or possible work around?

PovilasSlekys commented 5 years ago

It seems the apollo team is disinterested in this so don't expect a solution any time soon. As for a work around - don't use useLazyQuery :) I managed to change all of my Lazy queries into normal queries to avoid this and everything works for me now. I think it might be worthwhile for you to check if you can do this as well.

vilvaathibanpb commented 5 years ago

I found a solution but to implement it, I need clarity on other code change which was merged earlier from a core member. But I am still waiting for response

hwillson commented 5 years ago

Hi all - sorry for the delay here. We're heads down working on Apollo Client 3, which is going to supersede the React Apollo project (React Hooks functionality will be available from AC 3 directly). This unfortunately means we haven't had a chance to keep an eye on issues as much as we'd like 🙁. That being said, if I revert https://github.com/apollographql/react-apollo/pull/3497 and publish a new RA version today, would that be a good stop gap for now?

Rolandkuku commented 5 years ago

Would be nice, thanks.

hwillson commented 5 years ago

Rolled back and deployed in 3.1.3 - thanks!

santiagoalmeidabolannos commented 4 years ago

What about this? It keeps happening to me, what can I do for now?

RuslanZavacky commented 4 years ago

As a workaround, I've found a use of useQuery with skip property to work well. Something like this:

const [skipQuery, setSkipQuery] = useState(true);

let { loading } = useQuery(MyQuery, {
  client: myClient,
  variables: { var1, var2 },
  skip: skipQuery,
  onCompleted: data => {
    setData(data);
    setSkipQuery(true);
  },
});

useEffect(() => {
  setSkipQuery(false);

  return () => setSkipQuery(true);
}, [props.dep1, props.dep2]);

I hope that helps.

futantan commented 4 years ago

this issue still happens in version 3.1.3

MarkPolivchuk commented 4 years ago

@hwillson Ran into this on 3.1.3 today.

dylanwulf commented 4 years ago

@MarkPolivchuk @futantan Please open a new issue with a runnable reproduction if you're still having trouble with this. Also maybe take a look at @apollo/client v3 beta, there have been a lot of changes and there's a chance your issue has been fixed already. But again, if you're still having trouble, please provide a runnable reproduction

arindamINT commented 4 years ago

My API is not calling 2nd time when I am setting the compareItems value [];

const Compare = (props) => {
    const [compareItems, setCProducts] = useState([]);
    const [loadCompareData, { called, loading, data }] = useLazyQuery(
        GET_COMPARE_PRODUCT_BY_FILTER, {
        onCompleted: data => {
            console.log('data ', data);
            setCProducts(data.products.items);
        }
    }
    );

    useEffect(() => {
        const cProducts = JSON.parse(window.localStorage.getItem('compareProducts'));
        const sku = cProducts.map(item => item._sku);
        console.log('useEffect called', sku);
        loadCompareData({ variables: { sku }, refetch: { sku } });

    }, [compareItems]);

   // --- remove compareItem ---
    const clearCompareItem = (indx) => {
        setCProducts([]);
    }
return (
    <>
      <h1>Hello World</h1>
    </>
  );
}
yairtal commented 4 years ago

I am having the same issue and the above solutions doesn't help my case

tomerzcod7 commented 4 years ago

Any fix for this yet? This keeps happening to us with @apollo/client v3 beta as well.

dhavaljardosh commented 4 years ago
 const [getComment, { loading, data }] = useLazyQuery(getCommentsQuery);
  useEffect(() => {
    getComment({
      variables: {
        input: {
          id: "5e5cb512e90bd40017385305",
        },
      },
    });
  }, []);

  if (data && data.getPost) {
    var allNewComments = data.getPost.comments;
    setAllComments(allNewComments);  // re-renders
  }

Not sure where I'm doing something wrong.

I want to call the query the 1st time automatically in useEffect and then manually in the functions.

chattling commented 4 years ago

@dhavaljardosh How about this:

const [getComment] = useLazyQuery(getCommentsQuery, {
  onCompleted: data => {
    setAllComments(data.getPost.comments) // re-renders
  }
});
  useEffect(() => {
    getComment({
      variables: {
        input: {
          id: "5e5cb512e90bd40017385305",
        },
      },
    });
  }, []);
arindamINT commented 4 years ago

I absolutely agree with @chattling. A bit addition from my point.

const [getComment] = useLazyQuery(getCommentsQuery, {
    onCompleted: data => {
      setAllComments(data.getPost.comments) // re-renders
    },
    onError: ({ graphQLErrors, networkError }) => {
        if (graphQLErrors) {
            if (graphQLErrors.length > 0) console.log(graphQLErrors[0].message, 'error');
        } else if (networkError) {
            console.log('Please check your network connection!', 'error');
        }
    },
  });
useEffect(() => {
    getComment({
        variables: {
            input: {
                id: "5e5cb512e90bd40017385305",
            },
        },
    });
}, []);
Phoenix1405 commented 4 years ago

This seems to still be happening in 3.1.5 if I do useState and useLazyQuery together.

export const FirstApp = () => {
     const [orderNumber, setOrderNumber] = useState(null);
    const [ getOrderNumber , {loading, data}] = useLazyQuery(GET_ORDER_NUMBER);
    ...
    if (data && data.orderNumber) {
        setOrderNumber(data.orderNumber)
    }
    return (...) // someplace use { orderNumber } and trigger getOrderNumber using a button.
}
chattling commented 4 years ago

@Phoenix1405, the working design pattern would be to set state inside onCompleted event and you don't need to destructure loading and data, see example above.

arindamINT commented 4 years ago

This seems to still be happening in 3.1.5 if I do useState and useLazyQuery together.

keep a console.log inside the useEffect and onCompleted. Now observe whether the useEffect is firing multiple time or OnComplete.

If onCompleted log firing multiple times then the issue is somewhere else.

riturajratan commented 4 years ago

when the variables are same in useLazyQuery it will not call onCompleted but if variable is different then its call onCompleted again like in this example

  const [verifyUser] = useLazyQuery(GET_USER_BY_PHONE, {
    onCompleted: data => {
     // come when input is different
    },
  });

  verifyUser({variables: {filter: {phone: 1234567890}}});   // onCompleted will call
  verifyUser({variables: {filter: {phone: 1234567890}}});   // onCompleted will not call because same phone number
 verifyUser({variables: {filter: {phone: 11111111111}}}); // onCompleted will call again because phone number changed

so for this try fetchPolicy: 'network-only' Now every time its call onCompleted so final code would be

const [verifyUser] = useLazyQuery(GET_USER_BY_PHONE, {
   fetchPolicy: 'network-only'
    onCompleted: data => {
     // come everytime when you call verifyUser
    },
  });
LucasClaude commented 4 years ago

This worked for me as a workaround:

const [skipQuery, setSkipQuery] = useState(true);

let { loading, error, data } = useQuery(QUERY, {
    variables: { ...variables },
    skip: skipQuery,
});

useEffect(() => {
    if (!skipQuery) {
        const onCompleted = () => {};
        const onError = () => {};
        if (onCompleted || onError) {
            if (onCompleted && !loading && !error) {
                //SuccessFunctionHere
                setSkipQuery(true);
            }
            else if (onError && !loading && error) {
                //ErrorFunctionHere
                setSkipQuery(true);
            }
        }
    }
}, [loading, data, error]);
squarewave24 commented 4 years ago

run into a similar issue where data is present on every re-render after click. so my download gets triggered every time :(

export function DownloadTestData(props) {
  const [getTestData, { loading, error, data }] = useLazyQuery(QUERY_TEST);
  if (data)
     saveAsCsv(data)

  return <Button onClick={getTestData}>export</Button>
LucasClaude commented 4 years ago

run into a similar issue where data is present on every re-render after click. so my download gets triggered every time :(

export function DownloadTestData(props) {
  const [getTestData, { loading, error, data }] = useLazyQuery(QUERY_TEST);
  if (data)
     saveAsCsv(data)

  return <Button onClick={getTestData}>export</Button>

Try my above solution, I was having the same issue with useLazyQuery. Or simply check if there is data in a useEffect. Currently the if statement will get called on every re-render, where a useEffect with data as a dependency will only update when data is updated.

squarewave24 commented 4 years ago

@LucasClaude hey thanks yeah that trick works for me too. it just seems wrong to have to use a extra flag and useEffects. seems the whole premise of useLazyQuery would be to use it with a button. how does it work for anyone else? the example doesn't have any workarounds like this so seems like a design flaw? or i am misunderstanding hooks :)

arindamINT commented 4 years ago

UseEffect will be called for any state change. So you just have to know first what is your requirements. Generally useLazyQuery use for late loading purpose.