[RFC] Using hooks for fetch side effects

[fzaninotto]

Problem

We we wrote react-admin, there was no common solution for handling side effects (like redirection, notification, or refresh after the dataProvider answers). So we've chosen redux-saga, and declared side effects as objects in the meta field of redux actions, as follows:

export const crudUpdate = (
    resource: string,
    id: Identifier,
    data: any,
    previousData: any,
    basePath: string,
    redirectTo: RedirectionSideEffect = 'show'
): CrudUpdateAction => ({
    type: CRUD_UPDATE,
    payload: { id, data, previousData },
    meta: {
        resource,
        fetch: UPDATE,
        onSuccess: {
            notification: {
                body: 'ra.notification.updated',
                level: 'info',
                messageArgs: {
                    smart_count: 1,
                },
            },
            redirectTo,
            basePath,
        },
        onFailure: {
            notification: {
                body: 'ra.notification.http_error',
                level: 'warning',
            },
        },
    },
});

In v3, Following RFC #2952, we've removed the side effects from the action object to let them be decided by the caller. The current version in next looks like this:

    // in ra-core/src/controllers/useEditController.ts
    const [mutate, { loading: isSaving }] = useMutation(
        { type: 'EDIT', resource, payload: {} } // payload is set later by the caller of mutate
    );

    const save = (data, redirectTo = 'list') =>
        mutate(
            null, // the mutate callback ignores the first argument as it's usually an event sent by onSubmit or onClick
            { data }, // this overrides the mutation payload
            {
                onSuccess: {
                    notification: {
                        body: 'ra.notification.updated',
                        level: 'info',
                        messageArgs: {
                            smart_count: 1,
                        },
                    },
                    basePath,
                    redirectTo,
                },
                onFailure: {
                    notification: {
                        body: 'ra.notification.http_error',
                        level: 'warning',
                    },
                },
            },
        );
    );

So we've abstracted the action away (in the useMutation hook) but not the side effects.

I find these side effect objects to be an unnecessary level of indirection. They are hard to write, they require constant lookups in the documentation for syntax, they are hard to explain. Is the error side effect key 'onError' or 'onFailure'? I always have to check the right syntax.

And React 16.8 now offers a native way to handle side effects: hooks.

Proposed Solution

The useMutation hook uses the useDataProvider hook. I've designed useDataProvider to return a Promise, so that it's easy for the developer to execute code once the call to the data provider ends. But that ability wasn't kept in the current useMutation implementation. If we keep that ability (returning a Promise), then side effects can be added naturally:

    // in ra-core/src/controllers/useEditController.ts
    const [mutate, { loading: isSaving }] = useMutation(
        { type: 'EDIT', resource, payload: {} } // payload is set later by the caller of mutate
    );

    const save = (data, redirectTo = 'list') =>
        mutate(null, { data })
            .then(() => {
                // success side effects go here
            })
            .catch(() => {
                // failure side effects go here
            }),

But then, how to trigger these side effects? Let's create hooks for notification, redirection, and refresh.

    // in ra-core/src/controllers/useCreatecontroller.ts
+   const notify = useNotification();
+   const redirect = useRedirection();
    const [mutate, { loading: isSaving }] = useMutation(
        { type: 'EDIT', resource, payload: {} } // payload is set later by the caller of mutate
    );

    const save = (data, redirectTo = 'list') =>
        mutate(null, { data })
            .then(() => {
+               notify('ra.notification.created', 'info', {
+                   messageArgs: {
+                       smart_count: 1,
+                   },
+               });
+               redirect(redirectTo, basePath);
            })
            .catch(() => {
+               notify('ra.notification.http_error', 'warning');
            });

Drawback

While the changes to useMutation and the implementation of the new hooks aren't very hard, a problem occurs with useQuery. That's because this hook does not return a callback as useMutation, so we have nothing to attach a Promise to.

// current hooks signatures
const { data, total, loading, error } = useQuery({ type, resource, payload });
const [mutate, { data, total, loading, error }] = useMutation({ type, resource, payload });

We need a way to trigger side effects for read queries, too (e.g. redirecting to the list page when a request for a record in the edit page returns a 404).

So I suggest we change the signatures of both useQuery and useMutation to always return the query, which is a Promise:

// proposed hooks signatures
const { query,  data, total, loading, error } = useQuery({ type, resource, payload });
const { mutate, data, total, loading, error } = useMutation({ type, resource, payload });

That way, it becomes possible to attach success and failure side effects to read queries, too:

// in useEditController.js
// before
const { data: record, loading } = useGetOne(resource, id, {
    basePath,
    version, // used to force reload
    onFailure: {
        notification: {
            body: 'ra.notification.item_doesnt_exist',
            level: 'warning',
        },
        redirectTo: 'list',
        refresh: true,
    },
});

// after
const notify = useNotification();
const redirect = useRedirection();
const refresh = useRefresh();
const { query, data: record, loading } = useGetOne(resource, id, { version })
query.catch(() => {
    notify('ra.notification.item_doesnt_exist', 'warning');
    redirect('list', basePath);
    refresh();
})

Considering we're still in the alpha phase, I think we can afford changing these signatures.

Benefits

• the hooks are totally idiomatic in react 16.8. This feels like "just react" instad of something weird.
• We don't need to explain metas and sagas anymore. People don't need to open a dozen files to understand how that works.
• it's easy to add custom side effects
• the new side effects hooks can be used elsewhere (not only in request callbacks)

What's your take on that change?

[djhi]

I love it 😍

[alexisjanvier]

Wouldn't it be even more readable to return a query error? (a boolean or a more explicite object)

const { error, data: record, loading } = useGetOne(resource, id, { version })

Then use the useEffect to manage any notify, redirect or refresh.
This would hide the very relative complexity of query.catch in the useGetOne hook.

[fzaninotto]

The query hooks (useGetOne and useQuery alike) already return an error, which is null unless the data provider returned an error. But it doesn't allow to trigger a side effect when the error is received, because we never know how many times React renders a given component.

[alexisjanvier]

Okay, I think I understand.

[fzaninotto]

There is something wrong about the API I suggest, and that's catch().

• In a JS Promise, adding a catch() allows to catch an error. If catch isn't present and an error occurs, that error bubbles up.
• In my suggested API, adding a catch() allows to add an error side effect. If catch isn't present and an error occurs, nothing should happen (the case is already handled by the error field).

That means that using a JS Promise for side effects is not possible. We could implement a fake Promise that does nothing when an error occurs and no catch is attached, but this fake Promise would have slightly different semantics than the JS Promise.catch(), and would probably confuse developers.

I'm still scratching my head looking for a better alternative.

[fzaninotto]

The best I can come up with is to return functions to call for success and error side effects.

// proposed hooks signatures
const { data, total, loading, error, onSuccess, onError } = useQuery({ type, resource, payload });
const [mutate, { data, total, loading, error, onSuccess, onError }] = useMutation({ type, resource, payload });

So the syntax for side effects looks like the following for read queries:

// in useEditController.js
// before
const { data: record, loading } = useGetOne(resource, id, {
    basePath,
    version, // used to force reload
    onFailure: {
        notification: {
            body: 'ra.notification.item_doesnt_exist',
            level: 'warning',
        },
        redirectTo: 'list',
        refresh: true,
    },
});

// after
const notify = useNotification();
const redirect = useRedirection();
const refresh = useRefresh();
const { data: record, loading, onError } = useGetOne(resource, id, { version })
onError(() => {
    notify('ra.notification.item_doesnt_exist', 'warning');
    redirect('list', basePath);
    refresh();
})

And for mutations:

// in ra-core/src/controllers/useCreatecontroller.ts
const notify = useNotification();
const redirect = useRedirection();
const [mutate, { loading: isSaving, onSuccess, onError }] = useMutation(
    { type: 'EDIT', resource, payload: {} } // payload is set later by the caller of mutate
);

const save = (data, redirectTo = 'list') => {
    mutate(null, { data });
    onSuccess(() => {
        notify('ra.notification.created', 'info', {
            messageArgs: {
                smart_count: 1,
            },
        });
        redirect(redirectTo, basePath);
    });
    onError(() => {
        notify('ra.notification.http_error', 'warning');
    });
}

[fzaninotto]

In fact, the syntax I just proposed above is not really better than another syntax that is closer to the current one, and also allows both hook-based side effects and custom side effects:

// in useEditController.js
// before
const { data: record, loading } = useGetOne(resource, id, {
    basePath,
    version, // used to force reload
    onFailure: {
        notification: {
            body: 'ra.notification.item_doesnt_exist',
            level: 'warning',
        },
        redirectTo: 'list',
        refresh: true,
    },
});

// after
const notify = useNotification();
const redirect = useRedirection();
const refresh = useRefresh();
const { data: record, loading } = useGetOne(resource, id, { 
    version,  // used to force reload
    onFailure: () => {
        notify('ra.notification.item_doesnt_exist', 'warning');
        redirect('list', basePath);
        refresh();
    }
})

Straightfoward, isn't it?

And for mutations:

// in ra-core/src/controllers/useCreatecontroller.ts
const notify = useNotification();
const redirect = useRedirection();
const [mutate, { loading: isSaving, onSuccess, onError }] = useMutation(
    { type: 'EDIT', resource, payload: {} } // payload is set later by the caller of mutate
);

const save = (data, redirectTo = 'list') => {
    mutate(null, { data }, {
        onSuccess: () => {
            notify('ra.notification.created', 'info', {
                messageArgs: {
                    smart_count: 1,
                },
            });
            redirect(redirectTo, basePath);
        },
        onError: () => {
            notify('ra.notification.http_error', 'warning');
        }
    });
}

Implementing this last proposed syntax is much more natural than implementing the previous syntax, which implies useRef and hard to read plumbing (see below the proof of concept for the old syntax).

modified useQuery to return onSuccess and onError callbacks (sigh)

import { useEffect, useRef } from 'react';

import { useSafeSetState } from '../util/hooks';
import useDataProvider from './useDataProvider';

export interface Query {
    type: string;
    resource: string;
    payload: object;
}

export interface QueryOptions {
    meta?: any;
    action?: string;
    undoable?: false;
}

export type SideEffect = (args: any) => void;

const noop: SideEffect = () => {};

/**
* Fetch the data provider through Redux
*
* The return value updates according to the request state:
*
* - start: { query: [query Promise], loading: true, loaded: false }
* - success: { data: [data from response], total: [total from response], query: [query Promise], loading: false, loaded: true }
* - error: { error: [error from response], query: [query Promise], loading: false, loaded: true }
*
* @param {Object} query
* @param {string} query.type The verb passed to th data provider, e.g. 'GET_LIST', 'GET_ONE'
* @param {string} query.resource A resource name, e.g. 'posts', 'comments'
* @param {Object} query.payload The payload object, e.g; { post_id: 12 }
* @param {Object} options
* @param {string} options.action Redux action type
* @param {Object} options.meta Redux action metas, including side effects to be executed upon success of failure, e.g. { onSuccess: { refresh: true } }
*
* @returns The current request state. Destructure as { data, total, error, query, loading, loaded }.
*
* @example
* // basic usage
* import { useQuery } from 'react-admin';
*
* const UserProfile = ({ record }) => {
*     const { data, loading, error } = useQuery({
*         type: 'GET_ONE',
*         resource: 'users',
*         payload: { id: record.id }
*     });
*     if (loading) { return <Loading />; }
*     if (error) { return <p>ERROR</p>; }
*     return <div>User {data.username}</div>;
* };
*
* @example
* // usage of both ddata and total for GET_LIST
* import { useQuery } from 'react-admin';
*
* const payload = {
*    pagination: { page: 1, perPage: 10 },
*    sort: { field: 'username', order: 'ASC' },
* };
* const UserList = () => {
*     const { data, total, loading, error } = useQuery({
*         type: 'GET_LIST',
*         resource: 'users',
*         payload
*     });
*     if (loading) { return <Loading />; }
*     if (error) { return <p>ERROR</p>; }
*     return (
*         <div>
*             <p>Total users: {total}</p>
*             <ul>
*                 {data.map(user => <li key={user.username}>{user.username}</li>)}
*             </ul>
*         </div>
*     );
* };
*
* @example
* // attaching side effects
* import { useQuery, useNotification } from 'react-admin';
*
* const UserProfile = ({ record }) => {
*     const notify = useNotification();
*     const { data, loading, error, onError } = useQuery({
*         type: 'GET_ONE',
*         resource: 'users',
*         payload: { id: record.id }
*     });
*     onError(() => notify('User does not exist', 'warning'));
*     if (loading) { return <Loading />; }
*     if (error) { return <p>ERROR</p>; }
*     return <div>User {data.username}</div>;
* };
*/
const useQuery = (
    query: Query,
    options: QueryOptions = {}
): {
    data?: any;
    total?: number;
    error?: any;
    loading: boolean;
    loaded: boolean;
    onSuccess: SideEffect;
    onError: SideEffect;
} => {
    const { type, resource, payload } = query;
    let resolve = useRef(noop);
    let reject = useRef(noop);
    const onSuccess = res => (resolve.current = res);
    const onError = rej => (reject.current = rej);
    const [state, setState] = useSafeSetState({
        data: undefined,
        error: null,
        total: null,
        loading: true,
        loaded: false,
        onSuccess,
        onError,
    });
    const dataProvider = useDataProvider();
    useEffect(() => {
        dataProvider(type, resource, payload, options)
            .then(({ data, total }) => {
                setState({
                    data,
                    total,
                    loading: false,
                    loaded: true,
                    onSuccess: callback => callback({ data, total }),
                    onError: noop,
                });
                resolve.current({ data, total });
            })
            .catch(error => {
                setState({
                    error,
                    loading: false,
                    loaded: false,
                    onSuccess: noop,
                    onError: callback => callback(error),
                });
                reject.current(error);
            });
        // deep equality, see https://github.com/facebook/react/issues/14476#issuecomment-471199055
    }, [JSON.stringify({ query, options })]); // eslint-disable-line react-hooks/exhaustive-deps

    return state;
};

export default useQuery;

[ThieryMichel]

Another possibility to handle the additional configuration, would be to use a configurable function (like in d3).
something like:

const notify = useNotification();
const redirect = useRedirection();
const refresh = useRefresh();
const { data: record, loading } = useGetOne()
    .onFailure(() => {
        notify('ra.notification.item_doesnt_exist', 'warning');
        redirect('list', basePath);
        refresh();
    })(resource, id, { 
    version,  // used to force reload
})

This could also make the big configuration object more manageable, as we could add method for each configurable property, not just onSuccess and onFailure.
The tricky part is to isolate the hook configuration between component without breaking the rule of hook nor reconfiguring the function at every render.
I will give it a try.

[fzaninotto]

Fixed by #3425