Refactor middlewares to handle optimistic cases

packages/ra-core/src/controller/create/useCreateController.ts

@@ -79,7 +79,7 @@ export const useCreateController = <
         mutationOptions;
     const {
         registerMutationMiddleware,
-        mutateWithMiddlewares,
+        getMutateWithMiddlewares,
         unregisterMutationMiddleware,
     } = useMutationMiddlewares();
 
@@ -134,7 +134,7 @@ export const useCreateController = <
         },
         ...otherMutationOptions,
         returnPromise: true,
-        mutateWithMiddlewares,
+        getMutateWithMiddlewares,
     });
 
     const save = useCallback(

packages/ra-core/src/controller/edit/useEditController.ts

@@ -91,7 +91,7 @@ export const useEditController = <
     } = mutationOptions;
     const {
         registerMutationMiddleware,
-        mutateWithMiddlewares,
+        getMutateWithMiddlewares,
         unregisterMutationMiddleware,
     } = useMutationMiddlewares();
     const {
@@ -192,7 +192,7 @@ export const useEditController = <
             ...otherMutationOptions,
             mutationMode,
             returnPromise: mutationMode === 'pessimistic',
-            mutateWithMiddlewares,
+            getMutateWithMiddlewares,
         }
     );
 

packages/ra-core/src/controller/saveContext/useMutationMiddlewares.ts

@@ -51,12 +51,11 @@ export const useMutationMiddlewares = <
         []
     );
 
-    const mutateWithMiddlewares = useCallback(
-        (
-            fn: MutateFunc,
-            ...args: Parameters<MutateFunc>
-        ): ReturnType<MutateFunc> => {
-            let index = callbacks.current.length - 1;
+    const getMutateWithMiddlewares = useCallback((fn: MutateFunc) => {
+        // Stores the current callbacks in a closure to avoid losing them if the calling component is unmounted
+        const currentCallbacks = [...callbacks.current];
+        return (...args: Parameters<MutateFunc>): ReturnType<MutateFunc> => {
+            let index = currentCallbacks.length - 1;
 
             // Called by middlewares to call the next middleware function
             // Should take the same arguments as the original mutation function
@@ -67,32 +66,31 @@ export const useMutationMiddlewares = <
 
                 // If there are no more middlewares, we call the original mutation function
                 if (index >= 0) {
-                    return callbacks.current[index](...newArgs, next);
+                    return currentCallbacks[index](...newArgs, next);
                 } else {
                     return fn(...newArgs);
                 }
             };
 
-            if (callbacks.current.length > 0) {
+            if (currentCallbacks.length > 0) {
                 // Call the first middleware with the same args as the original mutation function
                 // with an additional next function
-                return callbacks.current[index](...args, next);
+                return currentCallbacks[index](...args, next);
             }
 
             return fn(...args);
-        },
-        []
-    );
+        };
+    }, []);
 
     const functions = useMemo<UseMutationMiddlewaresResult<MutateFunc>>(
         () => ({
             registerMutationMiddleware,
-            mutateWithMiddlewares,
+            getMutateWithMiddlewares,
             unregisterMutationMiddleware,
         }),
         [
             registerMutationMiddleware,
-            mutateWithMiddlewares,
+            getMutateWithMiddlewares,
             unregisterMutationMiddleware,
         ]
     );
@@ -104,10 +102,9 @@ export interface UseMutationMiddlewaresResult<
     MutateFunc extends (...args: any[]) => any = (...args: any[]) => any,
 > {
     registerMutationMiddleware: (callback: Middleware<MutateFunc>) => void;
-    mutateWithMiddlewares: (
-        mutate: MutateFunc,
-        ...args: Parameters<MutateFunc>
-    ) => ReturnType<MutateFunc>;
+    getMutateWithMiddlewares: (
+        mutate: MutateFunc
+    ) => (...args: Parameters<MutateFunc>) => ReturnType<MutateFunc>;
     unregisterMutationMiddleware: (callback: Middleware<MutateFunc>) => void;
 }
 

packages/ra-core/src/controller/saveContext/useRegisterMutationMiddleware.spec.tsx

@@ -0,0 +1,102 @@
+import React from 'react';
+import { fireEvent, render, screen, waitFor } from '@testing-library/react';
+import {
+    SaveContextProvider,
+    useMutationMiddlewares,
+    useRegisterMutationMiddleware,
+} from '../..';
+
+describe('useRegisterMutationMiddleware', () => {
+    it('should register and unregister middlewares correctly', async () => {
+        const middleware = jest.fn((next: () => void) => next());
+        const save = jest.fn();
+        const Middleware = () => {
+            useRegisterMutationMiddleware(middleware);
+            return null;
+        };
+        const Parent = () => {
+            const [mount, setMount] = React.useState(true);
+            const middlewaresFunctions = useMutationMiddlewares();
+
+            return (
+                <SaveContextProvider value={middlewaresFunctions}>
+                    {mount && <Middleware />}
+                    <button onClick={() => setMount(!mount)}>
+                        Toggle middleware
+                    </button>
+                    <button
+                        onClick={() => {
+                            const saveWithMiddlewares =
+                                middlewaresFunctions.getMutateWithMiddlewares(
+                                    save
+                                );
+                            saveWithMiddlewares();
+                        }}
+                    >
+                        Save
+                    </button>
+                </SaveContextProvider>
+            );
+        };
+
+        render(<Parent />);
+        fireEvent.click(screen.getByText('Save'));
+        await waitFor(() => {
+            expect(save).toHaveBeenCalledTimes(1);
+        });
+        expect(middleware).toHaveBeenCalledTimes(1);
+        save.mockClear();
+        middleware.mockClear();
+        fireEvent.click(screen.getByText('Toggle middleware'));
+        fireEvent.click(screen.getByText('Save'));
+        await waitFor(() => {
+            expect(save).toHaveBeenCalledTimes(1);
+        });
+        expect(middleware).not.toHaveBeenCalled();
+    });
+
+    it('should execute middlewares registered even if they have been unregistered as an optimistic side effect', async () => {
+        const middleware = jest.fn((next: () => void) => next());
+        const save = jest.fn();
+        const Middleware = () => {
+            useRegisterMutationMiddleware(middleware);
+            return <span>Middleware</span>;
+        };
+        const Parent = () => {
+            const [mount, setMount] = React.useState(true);
+            const middlewaresFunctions = useMutationMiddlewares();
+
+            return (
+                <SaveContextProvider value={middlewaresFunctions}>
+                    {mount && <Middleware />}
+                    <button
+                        onClick={() => {
+                            const saveWithMiddlewares =
+                                middlewaresFunctions.getMutateWithMiddlewares(
+                                    save
+                                );
+                            // Mimic optimistic side effect such as redirect which would unregister the middleware
+                            setMount(false);
+                            setTimeout(() => {
+                                saveWithMiddlewares();
+                            }, 250);
+                        }}
+                    >
+                        Save
+                    </button>
+                </SaveContextProvider>
+            );
+        };
+
+        render(<Parent />);
+        fireEvent.click(screen.getByText('Save'));
+        await waitFor(() => {
+            expect(screen.queryByText('Middleware')).toBeNull();
+            expect(save).not.toHaveBeenCalled();
+        });
+        await waitFor(() => {
+            expect(save).toHaveBeenCalledTimes(1);
+        });
+        expect(middleware).toHaveBeenCalledTimes(1);
+    });
+});

packages/ra-core/src/dataProvider/useCreate.spec.tsx

@@ -7,6 +7,10 @@ import { testDataProvider } from './testDataProvider';
 import { useCreate } from './useCreate';
 import { useGetList } from './useGetList';
 import { CoreAdminContext } from '../core';
+import {
+    WithMiddlewaresError,
+    WithMiddlewaresSuccess,
+} from './useCreate.stories';
 
 describe('useCreate', () => {
     it('returns a callback that can be used with create arguments', async () => {
@@ -181,4 +185,49 @@ describe('useCreate', () => {
             expect(screen.queryByText('ghi')).not.toBeNull();
         });
     });
+
+    describe('middlewares', () => {
+        it('it accepts middlewares and displays result and success side effects when dataProvider promise resolves', async () => {
+            render(<WithMiddlewaresSuccess timeout={10} />);
+            screen.getByText('Create post').click();
+            await waitFor(() => {
+                expect(screen.queryByText('success')).toBeNull();
+                expect(
+                    screen.queryByText('Hello World from middleware')
+                ).toBeNull();
+                expect(screen.queryByText('mutating')).not.toBeNull();
+            });
+            await waitFor(() => {
+                expect(screen.queryByText('success')).not.toBeNull();
+                expect(
+                    screen.queryByText('Hello World from middleware')
+                ).not.toBeNull();
+                expect(screen.queryByText('mutating')).toBeNull();
+            });
+        });
+
+        it('it accepts middlewares and displays error and error side effects when dataProvider promise rejects', async () => {
+            jest.spyOn(console, 'error').mockImplementation(() => {});
+            render(<WithMiddlewaresError timeout={10} />);
+            screen.getByText('Create post').click();
+            await waitFor(() => {
+                expect(screen.queryByText('success')).toBeNull();
+                expect(screen.queryByText('something went wrong')).toBeNull();
+                expect(
+                    screen.queryByText('Hello World from middleware')
+                ).toBeNull();
+                expect(screen.queryByText('mutating')).not.toBeNull();
+            });
+            await waitFor(() => {
+                expect(screen.queryByText('success')).toBeNull();
+                expect(
+                    screen.queryByText('something went wrong')
+                ).not.toBeNull();
+                expect(
+                    screen.queryByText('Hello World from middleware')
+                ).toBeNull();
+                expect(screen.queryByText('mutating')).toBeNull();
+            });
+        });
+    });
 });