fix(core): do not call mutate callbacks if mutation started after unmount (#4848)

janlat · TkDodo · web-flow · commit 901e826f394d · 2023-01-22T16:56:54.000+01:00
* test: add mutation callback after unmount test

* test: make test more resilient

* fix(core): do not call mutate callbacks if mutation started after unmount

* test: adapt tests to what we have in v5

- one test has been removed (because setState was removed entirely)
- the second test has been re-written to not use internals anymore, and it works in v4 as well

* fix(core): do not call mutate callbacks if mutation started after unmount

by making sure the callbacks are only invoked if we have an active listener

* chore: prettier again

Co-authored-by: Dominik Dorfmeister &lt;office@dorfmeister.cc&gt;
diff --git a/packages/query-core/src/mutationObserver.ts b/packages/query-core/src/mutationObserver.ts
@@ -166,7 +166,7 @@ export class MutationObserver<
   private notify(options: NotifyOptions) {
     notifyManager.batch(() => {
       // First trigger the mutate callbacks
-      if (this.mutateOptions) {
+      if (this.mutateOptions && this.hasListeners()) {
         if (options.onSuccess) {
           this.mutateOptions.onSuccess?.(
             this.currentResult.data!,
diff --git a/packages/query-core/src/tests/mutations.test.tsx b/packages/query-core/src/tests/mutations.test.tsx
@@ -308,49 +308,24 @@ describe('mutations', () => {
     expect(onSettled).toHaveBeenCalled()
   })
 
-  test('setState should update the mutation state', async () => {
-    const mutation = new MutationObserver(queryClient, {
-      mutationFn: async () => {
-        return 'update'
-      },
-      onMutate: (text) => text,
-    })
-    await mutation.mutate()
-    expect(mutation.getCurrentResult().data).toEqual('update')
-
-    // Force setState usage
-    // because no use case has been found using mutation.setState
-    const currentMutation = mutation['currentMutation']
-    currentMutation?.setState({
-      context: undefined,
-      variables: undefined,
-      data: 'new',
-      error: undefined,
-      failureCount: 0,
-      failureReason: null,
-      isPaused: false,
-      status: 'success',
-    })
+  test('addObserver should not add an existing observer', async () => {
+    const mutationCache = queryClient.getMutationCache()
+    const observer = new MutationObserver(queryClient, {})
+    const currentMutation = mutationCache.build(queryClient, {})
 
-    expect(mutation.getCurrentResult().data).toEqual('new')
-  })
+    const fn = jest.fn()
 
-  test('addObserver should not add an existing observer', async () => {
-    const mutation = new MutationObserver(queryClient, {
-      mutationFn: async () => {
-        return 'update'
-      },
-      onMutate: (text) => text,
+    const unsubscribe = mutationCache.subscribe((event) => {
+      fn(event.type)
     })
-    await mutation.mutate()
 
-    // Force addObserver usage to add an existing observer
-    // because no use case has been found
-    const currentMutation = mutation['currentMutation']!
-    expect(currentMutation['observers'].length).toEqual(1)
-    currentMutation.addObserver(mutation)
+    currentMutation.addObserver(observer)
+    currentMutation.addObserver(observer)
 
-    expect(currentMutation['observers'].length).toEqual(1)
+    expect(fn).toHaveBeenCalledTimes(1)
+    expect(fn).toHaveBeenCalledWith('observerAdded')
+
+    unsubscribe()
   })
 
   test('mutate should throw an error if no mutationFn found', async () => {
@@ -367,4 +342,20 @@ describe('mutations', () => {
     }
     expect(error).toEqual('No mutationFn found')
   })
+
+  test('mutate update the mutation state even without an active subscription', async () => {
+    const onSuccess = jest.fn()
+    const onSettled = jest.fn()
+
+    const mutation = new MutationObserver(queryClient, {
+      mutationFn: async () => {
+        return 'update'
+      },
+    })
+
+    await mutation.mutate(undefined, { onSuccess, onSettled })
+    expect(mutation.getCurrentResult().data).toEqual('update')
+    expect(onSuccess).not.toHaveBeenCalled()
+    expect(onSettled).not.toHaveBeenCalled()
+  })
 })
diff --git a/packages/react-query/src/__tests__/useMutation.test.tsx b/packages/react-query/src/__tests__/useMutation.test.tsx
@@ -1044,7 +1044,7 @@ describe('useMutation', () => {
     )
   })
 
-  test('should go to error state if onSuccess callback errors', async () => {
+  it('should go to error state if onSuccess callback errors', async () => {
     const error = new Error('error from onSuccess')
     const onError = jest.fn()
 
@@ -1079,7 +1079,7 @@ describe('useMutation', () => {
     expect(onError).toHaveBeenCalledWith(error, 'todo', undefined)
   })
 
-  test('should go to error state if onError callback errors', async () => {
+  it('should go to error state if onError callback errors', async () => {
     const error = new Error('error from onError')
     const mutateFnError = new Error('mutateFnError')
 
@@ -1115,7 +1115,7 @@ describe('useMutation', () => {
     await rendered.findByText('error: mutateFnError, status: error')
   })
 
-  test('should go to error state if onSettled callback errors', async () => {
+  it('should go to error state if onSettled callback errors', async () => {
     const error = new Error('error from onSettled')
     const mutateFnError = new Error('mutateFnError')
     const onError = jest.fn()
@@ -1154,4 +1154,60 @@ describe('useMutation', () => {
 
     expect(onError).toHaveBeenCalledWith(mutateFnError, 'todo', undefined)
   })
+
+  it('should not call mutate callbacks for mutations started after unmount', async () => {
+    const onSuccessMutate = jest.fn()
+    const onSuccessUseMutation = jest.fn()
+    const onSettledMutate = jest.fn()
+    const onSettledUseMutation = jest.fn()
+
+    function Page() {
+      const [show, setShow] = React.useState(true)
+      return (
+        <div>
+          <button onClick={() => setShow(false)}>hide</button>
+          {show && <Component />}
+        </div>
+      )
+    }
+
+    function Component() {
+      const mutation = useMutation({
+        mutationFn: async (text: string) => {
+          await sleep(10)
+          return text
+        },
+        onSuccess: onSuccessUseMutation,
+        onSettled: onSettledUseMutation,
+      })
+
+      return (
+        <div>
+          <button
+            onClick={() => {
+              setActTimeout(() => {
+                mutation.mutate('todo', {
+                  onSuccess: onSuccessMutate,
+                  onSettled: onSettledMutate,
+                })
+              }, 10)
+            }}
+          >
+            mutate
+          </button>
+        </div>
+      )
+    }
+
+    const rendered = renderWithClient(queryClient, <Page />)
+
+    fireEvent.click(rendered.getByRole('button', { name: /mutate/i }))
+    fireEvent.click(rendered.getByRole('button', { name: /hide/i }))
+
+    await waitFor(() => expect(onSuccessUseMutation).toHaveBeenCalledTimes(1))
+    await waitFor(() => expect(onSettledUseMutation).toHaveBeenCalledTimes(1))
+
+    expect(onSuccessMutate).toHaveBeenCalledTimes(0)
+    expect(onSettledMutate).toHaveBeenCalledTimes(0)
+  })
 })
