useMutation calls will resolve for consumers after internal state like loading has already been updated

[dannycochran]

Intended outcome:

Given a simple component:

export const SimpleModal = (props: SimpleModalProps) => {
  const { selection, onSuccess, open, onClose, onError } = props;
  const [deleteShots, { loading: deleteShotsLoading }] = useMutation(DELETE_SHOTS);
  const onConfirmDeleteSelected = useCallback(async () => {
    try {
      await deleteShots({ variables: { shotIds: Object.keys(selection.rowIds) } });
      onSuccess?.();
    } catch (error) {
      onError?.(error);
    }
  }, [selection, onSuccess, onError, deleteShots]);

  return (
    <Modal open={open} onClose={onClose}>
      <Button
        onClick={onConfirmDeleteSelected}
        disabled={deleteShotsLoading}
      >
        Delete Shots
      </PrimaryButton>
    </Modal>
  );
};

I would expect that after deleteShots succeeds, the internal state of the mutation has not yet updated. This is important since if it updates before I am able to take an action and the loading state becomes false, the button will actually become enabled again before I've called onSuccess.

In this contrived scenario, if a user clicks on the button fast enough in between the time that loading moves to false and onSuccess is called (which is closing the modal), they'll trigger multiple mutations which will eventually result in an error because the entity that was deleted no longer exists.

Actual outcome:

The internal state is updated first:
https://github.com/apollographql/apollo-client/blob/main/src/react/data/MutationData.ts#L73

So the loading state switches to "false", and then the consumer code is executed (e.g. onSuccess).

How to reproduce the issue:

I can reproduce if it's helpful but the above snippet should be sufficient, and I first want to make sure there's alignment on what the expected behavior should be before spending more time.

Workaround:

The obvious workaround is using my own separate loading state, but that feels a bit unfortunate:

  const [loading, setLoading] = useState(false);
  const onConfirmDeleteSelected = useCallback(async () => {
    setLoading(true);
    try {
      await deleteShots({ variables: { shotIds: Object.keys(selection.rowIds) } });
      onSuccess?.();
    } catch (error) {
      onError?.(error);
    } finally {
      setLoading(false);
    }
  }, [selection, onSuccess, onError, deleteShots]);

Possible Solutions:

If we were to wrap that call to onMutationCompleted in a setTimeout, the issue goes away, e.g:

    return this.mutate(mutationFunctionOptions)
      .then((response: FetchResult<TData>) => {
        setTimeout(() => this.onMutationCompleted(response, mutationId));
        return response;
      })

However this behavior would almost certainly cause surprises or new behavior for many consumers so I'm not sure if it should be the default. I also don't know what would be a sensible way to opt in to this behavior, it feels like a very weird feature, e.g. delayStateUpdate ?

Versions

  System:
    OS: macOS 11.2.2
  Binaries:
    Node: 12.18.4 - /usr/local/bin/node
    Yarn: 1.22.4 - ~/npm-global/bin/yarn
    npm: 6.14.6 - /usr/local/bin/npm
  Browsers:
    Chrome: 90.0.4430.93
    Safari: 14.0.3
  npmPackages:
    @apollo/client: ^3.3.15 => 3.3.15 
    apollo-upload-client: 14.1.3 => 14.1.3 
    apollo3-cache-persist: 0.9.1 => 0.9.1 
  npmGlobalPackages:
    apollo: 2.27.0

[brainkim]

@dannycochran Thanks for opening the issue. Some questions.

I would expect that after deleteShots succeeds, the internal state of the mutation has not yet updated. This is important since if it updates before I am able to take an action and the loading state becomes false, the button will actually become enabled again before I've called onSuccess.

Can you talk a little bit more about your expectations? What do you mean by “internal state” here? My guess is that you’re saying the {loading, data, errors} should not have been updated and caused a rerender by the time the promise settles? This feels reasonable to me, but overall my question with the useMutation API is why we have both a callback option API like onError and onCompleted AND a promise-based API, which encapsulates both callbacks? This is not really a question for you, but for myself haha.

they'll trigger multiple mutations which will eventually result in an error because the entity that was deleted no longer exists

Why is this an error condition? Can you somehow refactor the API so that it works like deleting an item from a set, i.e. make it idempotent? I think that would go a long way for reliability here.

---

I’m not opposed to deferring setState() calls in mutations til after the promise resolves. I’m not sure this is something which is well-defined and I personally have no strong opinions on which should happen first. If we do this, I would rather we avoid setTimeout in favor of maybe running separate callbacks which rerender React. Note that this would change the error handling logic, insofar as synchronous errors thrown in onCompleted callbacks are currently caught in the catch promise later.

[dannycochran]

Thanks for the response @brainkim !

Can you talk a little bit more about your expectations? What do you mean by “internal state” here? My guess is that you’re saying the {loading, data, errors} should not have been updated and caused a rerender by the time the promise settles? This feels reasonable to me, but overall my question with the useMutation API is why we have both a callback option API like onError and onCompleted AND a promise-based API, which encapsulates both callbacks? This is not really a question for you, but for myself haha.

That's correct, I'm referring to loading / data / errors. And you have it right, it will cause a re-render before the promise settles, so my component is now acting on that state, which in the code snippet above, means the button becomes enabled again. Once the promise resolves, it calls onSuccess which is the consumer callback, which ends up closing the modal. So basically it's an edge case where you can click super fast to cause multiple mutations.

Why is this an error condition? Can you somehow refactor the API so that it works like deleting an item from a set, i.e. make it idempotent? I think that would go a long way for reliability here.

We'd need to refactor in a lot of places. Basically we're passing an entity ID to a modal, and the modal is responsible for removing that ID. We don't have any knowledge of whether or not that entity still exists because we don't ever write to the cache ourselves. I could instead make the disabled state check if the ID is still in the cache, but that only works if we've previously fetched the entity. What if the modal is deleting entities we don't have fetched in the client? IMO I should be able to rely on the loading state to not re-render the component before the promise resolves, which will keep the button blocked. I also proposed another solution in my original comment where I maintain my own loading state, but that feels unfortunate and duplicative.

I’m not opposed to deferring setState() calls in mutations til after the promise resolves. I’m not sure this is something which is well-defined and I personally have no strong opinions on which should happen first. If we do this, I would rather we avoid setTimeout in favor of maybe running separate callbacks which rerender React. Note that this would change the error handling logic, insofar as synchronous errors thrown in onCompleted callbacks are currently caught in the catch promise later.

Yeah setTimeout was just an example to illustrate how you can defer but I agree we'd probably need something more robust. Maybe it could somehow be opt-in via deferMutationUpdates ? That isn't terribly descriptive but I'm not sure how to make it better.

[brainkim]

Maybe it could somehow be opt-in via deferMutationUpdates

I would personally be against any sort of new option, in favor of changing the behavior to match your expectations. Such an option would be very difficult to communicate.

[dannycochran]

Yep I generally agree. Seems like it could cause breaking changes for consumers who were somehow previously reliant on this behavior, but it seems like logical default behavior.

I’d volunteer for a PR here but probably wouldn’t be able to get to this for a while.

[dannycochran]

@brainkim another undesirable behavior here is that if you delete an entity ID, and that component has a "useQuery" which is getting information on that entityId, it will fire again before I have time to redirect to another page and prevent the "useQuery" from ever firing. Currently solve this by doing the redirect within update, e.g:

const result = useQuery(FOO, { id: entityId });

// elsewhere in the same component:

        await deleteEntity({
          variables: { id: entityId },
          update: () => {
            // TODO: If this gets resolved we can rely on redirecting
            // after the promise resolves: https://github.com/apollographql/apollo-client/issues/8137
            onSuccess?.();
            if (redirect) {
              history.push(routeWithoutEntityId());
            }
          },
        });