onComplete is invoked after rendering with loaded data in 3.8.x

[dfilatov]

Issue Description

Something has been broken in 3.8.x versions. onCompleted now is invoked after rendering with loaded data, it's unexpected and leads to errors in our application because we need to do some additional work with loaded data before it's rendered. In 3.7.17 version this works as expected.

Link to Reproduction

https://codesandbox.io/s/sad-fermat-djg7mq

Reproduction Steps

• open https://codesandbox.io/s/sad-fermat-djg7mq
• take a look at console, you'll see:
  render true
completed
render false
• change version of @apollo/client to 3.8.3, save
• take a look at console, you'll see:
  render true
render false
completed

[phryneas]

3.8 includes a change that is generally recommended by the React team when dealing with async data sources (like Apollo Client): It calls the handleStoreChange callback of useSyncExternalStore instead of a useState setState callback like before.
As a result, React not applies different timing of things here.

Generally, please note that we don't have a lot of control over the order of operation here - React decides pretty arbitrarily when it rerenders.
We actually have always called the code triggering onCompleted after we trigger the rerender. Until now, it caused the React state update batching behavior to move the next render back a tick. handleStoreChange should also do this (to batch with other external state source updates), but sometimes it doesn't.
Keep in mind, this is "autobatching" is only the case in React 18 - in React 17 you would see the current behaviour even with older versions of Apollo Client.

Once React fixes this bug the timing will likely go back to the behavior you have seen in React 18 with 3.7.

=> We have never been able to guarantee a certain order of operation, and can also not do so going into the future. React decides when it actually rerenders, not us.

Out of curiosity - what are you doing here that needs to happen before a render?

[dfilatov]

@phryneas Thank you for quick response!

Out of curiosity - what are you doing here that needs to happen before a render?

We are building/committing/uncommitting/resetting our internal models based on query data within onCompleted/onError

[phryneas]

Hm, if that is local component state, most of the time you should be able to do that synchronously during component render - can you share some example code here? Maybe I can suggest an alternative way of doing so.

[dfilatov]

Very simplified approach looks like:

const MyComponent = ({ model }) => {
    const { loading } = useSomeDataQuery({
        onCompleted(data) {
            model.setData(data);
            model.commit();
        }
    });

    return loading ?
        <Loader/> :
        <ModelView={ model }/>;        
};

I could get the same behaviour with React.useMemo but that looks quite weird and unnatural to me:

const MyComponent = ({ model }) => {
    const { data, loading } = useSomeDataQuery({});
    const modelWithData = React.useMemo(
        () => {
            if(loading) {
                return null;
            }

            model.setData(data);
            model.commit();

            return model;
        },
        [loading, data]
    );

    return loading ?
        <Loader/> :
        <ModelView={ modelWithData }/>;        
};

[iamnnort]

The same issue

[Mofo50C]

Yeah, im having this issue as well. Apollo client v3.8.8, react v18.2, nextjs v14. After a mutation, refetchQueries runs, query is updated, data is returned but onCompleted isn't called so the component state doesn't update. Previously working on 3.7

[ricardopieper]

Same, updated to Apollo v3.8.6 and this started happening. However I agree with the new behavior. If the onComplete handler took minutes to run, it shouldn't block the component from getting the data.