refactor: deprecate onMutate in favor of onBeforeMutate

People sometimes think onMutate triggers after the mutation and try to
invalidate queries there. Adding _before_ makes it clearer.

Author: posva

Diff for: src/mutation-options.ts

@@ -13,6 +13,16 @@ export interface UseMutationOptionsGlobal {
    * passed to `mutation`, `onSuccess`, `onError` and `onSettled`. If it
    * returns a promise, it will be awaited before running `mutation`.
    */
+  onBeforeMutate?: (
+    /**
+     * The variables passed to the mutation.
+     */
+    vars: unknown,
+  ) => _Awaitable<UseMutationGlobalContext | undefined | void | null>
+
+  /**
+   * @deprecated Use {@link onBeforeMutate} instead.
+   */
   onMutate?: (
     /**
      * The variables passed to the mutation.
@@ -33,7 +43,7 @@ export interface UseMutationOptionsGlobal {
      */
     vars: unknown,
     /**
-     * The merged context from `onMutate` and the global context.
+     * The merged context from `onBeforeMutate` and the global context.
      */
     context: UseMutationGlobalContext,
   ) => unknown
@@ -51,13 +61,13 @@ export interface UseMutationOptionsGlobal {
      */
     vars: unknown,
     /**
-     * The merged context from `onMutate` and the global context. Properties returned by `onMutate` can be `undefined`
-     * if `onMutate` throws.
+     * The merged context from `onBeforeMutate` and the global context. Properties returned by `onBeforeMutate` can be `undefined`
+     * if `onBeforeMutate` throws.
      */
     context:
       | Partial<Record<keyof UseMutationGlobalContext, never>>
       // this is the success case where everything is defined
-      // undefined if global onMutate throws
+      // undefined if global onBeforeMutate throws
       | UseMutationGlobalContext,
   ) => unknown
 
@@ -78,13 +88,13 @@ export interface UseMutationOptionsGlobal {
      */
     vars: unknown,
     /**
-     * The merged context from `onMutate` and the global context. Properties returned by `onMutate` can be `undefined`
-     * if `onMutate` throws.
+     * The merged context from `onBeforeMutate` and the global context. Properties are `undefined`
+     * if `onBeforeMutate` throws.
      */
     context:
       | Partial<Record<keyof UseMutationGlobalContext, never>>
       // this is the success case where everything is defined
-      // undefined if global onMutate throws
+      // undefined if global onBeforeMutate throws
       | UseMutationGlobalContext,
   ) => unknown
 
@@ -126,6 +136,17 @@ export interface UseMutationOptions<
    */
   key?: _MutationKey<NoInfer<TVars>>
 
+  /**
+   * @deprecated Use {@link onBeforeMutate} instead.
+   */
+  onMutate?: (
+    /**
+     * The variables passed to the mutation.
+     */
+    vars: NoInfer<TVars>,
+    context: UseMutationGlobalContext,
+  ) => _Awaitable<TContext | undefined | void | null>
+
   /**
    * Runs before the mutation is executed. **It should be placed before `mutation()` for `context` to be inferred**. It
    * can return a value that will be passed to `mutation`, `onSuccess`, `onError` and `onSettled`. If it returns a
@@ -136,7 +157,7 @@ export interface UseMutationOptions<
    * useMutation({
    * // must appear before `mutation` for `{ foo: string }` to be inferred
    * // within `mutation`
-   *   onMutate() {
+   *   onBeforeMutate() {
    *     return { foo: 'bar' }
    *   },
    *   mutation: (id: number, { foo }) => {
@@ -149,7 +170,7 @@ export interface UseMutationOptions<
    * })
    * ```
    */
-  onMutate?: (
+  onBeforeMutate?: (
     /**
      * The variables passed to the mutation.
      */
@@ -170,7 +191,7 @@ export interface UseMutationOptions<
      */
     vars: NoInfer<TVars>,
     /**
-     * The merged context from `onMutate` and the global context.
+     * The merged context from `onBeforeMutate` and the global context.
      */
     context: UseMutationGlobalContext & _ReduceContext<NoInfer<TContext>>,
   ) => unknown
@@ -188,14 +209,14 @@ export interface UseMutationOptions<
      */
     vars: NoInfer<TVars>,
     /**
-     * The merged context from `onMutate` and the global context. Properties returned by `onMutate` can be `undefined`
-     * if `onMutate` throws.
+     * The merged context from `onBeforeMutate` and the global context. Properties returned by `onBeforeMutate` can be `undefined`
+     * if `onBeforeMutate` throws.
      */
     context:
       | (Partial<Record<keyof UseMutationGlobalContext, never>> &
           Partial<Record<keyof _ReduceContext<NoInfer<TContext>>, never>>)
       // this is the success case where everything is defined
-      // undefined if global onMutate throws
+      // undefined if global onBeforeMutate throws
       | (UseMutationGlobalContext & _ReduceContext<NoInfer<TContext>>),
   ) => unknown
 
@@ -216,14 +237,14 @@ export interface UseMutationOptions<
      */
     vars: NoInfer<TVars>,
     /**
-     * The merged context from `onMutate` and the global context. Properties returned by `onMutate` can be `undefined`
-     * if `onMutate` throws.
+     * The merged context from `onBeforeMutate` and the global context. Properties returned by `onBeforeMutate` can be `undefined`
+     * if `onBeforeMutate` throws.
      */
     context:
       | (Partial<Record<keyof UseMutationGlobalContext, never>> &
           Partial<Record<keyof _ReduceContext<NoInfer<TContext>>, never>>)
       // this is the success case where everything is defined
-      // undefined if global onMutate throws
+      // undefined if global onBeforeMutate throws
       | (UseMutationGlobalContext & _ReduceContext<NoInfer<TContext>>),
   ) => unknown
 }

Diff for: src/mutation-store.ts

@@ -367,7 +367,7 @@ export const useMutationCache = /* @__PURE__ */ defineStore(MUTATION_STORE_ID, (
     let currentData: TResult | undefined
     let currentError: TError | undefined
     type OnMutateContext = Parameters<
-      Required<UseMutationOptions<TResult, TVars, TError, TContext>>['onMutate']
+      Required<UseMutationOptions<TResult, TVars, TError, TContext>>['onBeforeMutate']
     >['1']
     type OnSuccessContext = Parameters<
       Required<UseMutationOptions<TResult, TVars, TError, TContext>>['onSuccess']
@@ -378,24 +378,42 @@ export const useMutationCache = /* @__PURE__ */ defineStore(MUTATION_STORE_ID, (
 
     let context: OnMutateContext | OnErrorContext | OnSuccessContext = {}
 
+    if (process.env.NODE_ENV !== 'production') {
+      if (globalOptions.onMutate || options.onMutate) {
+        console.warn(
+          `[@pinia/colada] The "onMutate" option is deprecated. Use "onBeforeMutate" instead. It will be removed on the next version.`,
+        )
+        if (
+          (globalOptions.onMutate && globalOptions.onBeforeMutate)
+          || (options.onMutate && options.onBeforeMutate)
+        ) {
+          console.warn(
+            `[@pinia/colada] You are using both "onMutate" and "onBeforeMutate" but only "onMutate" is ran. Use only "onBeforeMutate" instead.`,
+          )
+        }
+      }
+    }
+
     try {
-      const globalOnMutateContext = globalOptions.onMutate?.(vars)
+      const globalOnMutateContext
+        = globalOptions.onMutate?.(vars) || globalOptions.onBeforeMutate?.(vars)
 
       context
         = (globalOnMutateContext instanceof Promise
           ? await globalOnMutateContext
           : globalOnMutateContext) || {}
 
-      const onMutateContext = (await options.onMutate?.(
-        vars,
-        context,
-        // NOTE: the cast makes it easier to write without extra code. It's safe because { ...null, ...undefined } works and TContext must be a Record<any, any>
-      )) as _ReduceContext<TContext>
+      const onBeforeMutateContext = (await (options.onMutate?.(vars, context)
+        || options.onBeforeMutate?.(
+          vars,
+          context,
+          // NOTE: the cast makes it easier to write without extra code. It's safe because { ...null, ...undefined } works and TContext must be a Record<any, any>
+        ))) as _ReduceContext<TContext>
 
       // we set the context here so it can be used by other hooks
       context = {
         ...context,
-        ...onMutateContext,
+        ...onBeforeMutateContext,
         // NOTE: needed for onSuccess cast
       } satisfies OnSuccessContext
 

Diff for: src/use-mutation.spec.ts

@@ -122,7 +122,24 @@ describe('useMutation', () => {
   })
 
   describe('hooks', () => {
-    it('invokes the "onMutate" hook before mutating', async () => {
+    it('invokes the "onBeforeMutate" hook before mutating', async () => {
+      const onBeforeMutate = vi.fn()
+      const { wrapper } = mountSimple({
+        mutation: async ({ a, b }: { a: number, b: number }) => {
+          return a + b
+        },
+        onBeforeMutate,
+      })
+      expect(onBeforeMutate).not.toHaveBeenCalled()
+      wrapper.vm.mutate({ a: 24, b: 42 })
+      expect(onBeforeMutate).toHaveBeenCalledTimes(1)
+      expect(onBeforeMutate).toHaveBeenLastCalledWith({ a: 24, b: 42 }, expect.objectContaining({}))
+      wrapper.vm.mutateAsync({ a: 0, b: 1 })
+      expect(onBeforeMutate).toHaveBeenCalledTimes(2)
+      expect(onBeforeMutate).toHaveBeenLastCalledWith({ a: 0, b: 1 }, expect.objectContaining({}))
+    })
+
+    it('works with the deprecated local "onMutate"', async () => {
       const onMutate = vi.fn()
       const { wrapper } = mountSimple({
         mutation: async ({ a, b }: { a: number, b: number }) => {
@@ -137,6 +154,22 @@ describe('useMutation', () => {
       wrapper.vm.mutateAsync({ a: 0, b: 1 })
       expect(onMutate).toHaveBeenCalledTimes(2)
       expect(onMutate).toHaveBeenLastCalledWith({ a: 0, b: 1 }, expect.objectContaining({}))
+      expect('"onMutate" option is deprecated').toHaveBeenWarned()
+    })
+
+    it('warns about misusing deprecated "onMutate"', async () => {
+      const onMutate = vi.fn()
+      const onBeforeMutate = vi.fn()
+      const { wrapper } = mountSimple({
+        mutation: async ({ a, b }: { a: number, b: number }) => {
+          return a + b
+        },
+        onMutate,
+        onBeforeMutate,
+      })
+      wrapper.vm.mutate({ a: 24, b: 42 })
+      expect('"onMutate" option is deprecated').toHaveBeenWarned()
+      expect('Use only "onBeforeMutate"').toHaveBeenWarned()
     })
 
     it('invokes the "onError" hook if mutation throws', async () => {
@@ -154,36 +187,36 @@ describe('useMutation', () => {
       expect(onError).toHaveBeenCalledWith(new Error('24'), 24, expect.objectContaining({}))
     })
 
-    it('invokes the "onError" hook if onMutate throws', async () => {
+    it('invokes the "onError" hook if onBeforeMutate throws', async () => {
       const onError = vi.fn()
       const { wrapper } = mountSimple({
-        onMutate() {
-          throw new Error('onMutate')
+        onBeforeMutate() {
+          throw new Error('onBeforeMutate')
         },
         onError,
       })
 
       wrapper.vm.mutate()
       await flushPromises()
       expect(onError).toHaveBeenCalledWith(
-        new Error('onMutate'),
+        new Error('onBeforeMutate'),
         undefined,
         expect.objectContaining({}),
       )
     })
 
-    it('passes the returned value from onMutate to onError', async () => {
+    it('passes the returned value from onBeforeMutate to onError', async () => {
       const onError = vi.fn()
       const { wrapper, mutation } = mountSimple({
-        onMutate: () => ({ foo: 'bar' }),
+        onBeforeMutate: () => ({ foo: 'bar' }),
         onError,
       })
 
-      mutation.mockRejectedValueOnce(new Error('onMutate'))
+      mutation.mockRejectedValueOnce(new Error('onBeforeMutate'))
       wrapper.vm.mutate()
       await flushPromises()
       expect(onError).toHaveBeenCalledWith(
-        new Error('onMutate'),
+        new Error('onBeforeMutate'),
         undefined,
         expect.objectContaining({ foo: 'bar' }),
       )
@@ -205,12 +238,12 @@ describe('useMutation', () => {
       expect(wrapper.vm.error).toEqual(null)
     })
 
-    it('awaits the "onMutate" hook before mutation', async () => {
-      const onMutate = vi.fn(async () => delay(10))
-      const { wrapper, mutation } = mountSimple({ onMutate })
+    it('awaits the "onBeforeMutate" hook before mutation', async () => {
+      const onBeforeMutate = vi.fn(async () => delay(10))
+      const { wrapper, mutation } = mountSimple({ onBeforeMutate })
 
       wrapper.vm.mutate()
-      expect(onMutate).toHaveBeenCalled()
+      expect(onBeforeMutate).toHaveBeenCalled()
       expect(mutation).not.toHaveBeenCalled()
       vi.advanceTimersByTime(10)
       expect(mutation).not.toHaveBeenCalled()
@@ -249,14 +282,14 @@ describe('useMutation', () => {
       expect(wrapper.vm.error).toEqual(new Error('onSuccess'))
     })
 
-    it('sets the error if "onMutate" throws', async () => {
-      const onMutate = vi.fn().mockRejectedValueOnce(new Error('onMutate'))
-      const { wrapper } = mountSimple({ onMutate })
+    it('sets the error if "onBeforeMutate" throws', async () => {
+      const onBeforeMutate = vi.fn().mockRejectedValueOnce(new Error('onBeforeMutate'))
+      const { wrapper } = mountSimple({ onBeforeMutate })
 
       wrapper.vm.mutate()
       await flushPromises()
-      expect(onMutate).toHaveBeenCalled()
-      expect(wrapper.vm.error).toEqual(new Error('onMutate'))
+      expect(onBeforeMutate).toHaveBeenCalled()
+      expect(wrapper.vm.error).toEqual(new Error('onBeforeMutate'))
     })
 
     describe('invokes the "onSettled" hook', () => {
@@ -296,37 +329,37 @@ describe('useMutation', () => {
       })
     })
 
-    it('triggers global onMutate', async () => {
-      const onMutate = vi.fn()
+    it('triggers global onBeforeMutate', async () => {
+      const onBeforeMutate = vi.fn()
       const { wrapper } = mountSimple(
         {},
         {
-          plugins: [createPinia(), [PiniaColada, { mutationOptions: { onMutate } }]],
+          plugins: [createPinia(), [PiniaColada, { mutationOptions: { onBeforeMutate } }]],
         },
       )
 
-      expect(onMutate).toHaveBeenCalledTimes(0)
+      expect(onBeforeMutate).toHaveBeenCalledTimes(0)
       wrapper.vm.mutate()
       // no need since it's synchronous
       // await flushPromises()
-      expect(onMutate).toHaveBeenCalledTimes(1)
-      expect(onMutate).toHaveBeenCalledWith(undefined)
+      expect(onBeforeMutate).toHaveBeenCalledTimes(1)
+      expect(onBeforeMutate).toHaveBeenCalledWith(undefined)
     })
 
-    it('local onMutate receives global onMutate result', async () => {
-      const onMutate = vi.fn(() => ({ foo: 'bar' }))
+    it('local onBeforeMutate receives global onBeforeMutate result', async () => {
+      const onBeforeMutate = vi.fn(() => ({ foo: 'bar' }))
       const { wrapper } = mountSimple(
-        { onMutate },
+        { onBeforeMutate },
         {
           plugins: [
             createPinia(),
-            [PiniaColada, { mutationOptions: { onMutate: () => ({ global: true }) } }],
+            [PiniaColada, { mutationOptions: { onBeforeMutate: () => ({ global: true }) } }],
           ],
         },
       )
 
       wrapper.vm.mutate()
-      expect(onMutate).toHaveBeenCalledWith(undefined, { global: true })
+      expect(onBeforeMutate).toHaveBeenCalledWith(undefined, { global: true })
     })
 
     it('triggers global onSuccess', async () => {