[RFR] Rely on meta to handle optimistic actions

[djhi]

Context: https://marmelab.com/react-admin/Actions.html#using-a-custom-action-creator

We were relying on the action type to handle optimistic effects. This prevent custom actions to be handled optimistically too.

Consider the following case: I have an ACCEPT_CHANGE and REJECT_CHANGE actions. Both will remove their targeted item from the change resource but will be caught by a custom saga to be applied on another resource. From the change resource perspective, they should be removed after being either accepted or rejected.

export const acceptChange = (
    id: string,
    previousData: any,
    basePath: string
) => ({
    type: ACCEPT_CHANGE,
    payload: { id, previousData },
    meta: {
        resource: 'changes',
        fetch: DELETE,
        onSuccess: {
            notification: {
                body: 'myapp.notification.accepted',
                level: 'info',
                messageArgs: {
                    smart_count: 1,
                },
            },
            redirectTo: 'list',
            basePath,
        },
        onFailure: {
            notification: {
                body: 'ra.notification.http_error',
                level: 'warning',
            },
        },
    },
});

[ThieryMichel]

Unless previousState[id] returns an array you can use a map here. And in this case you can do it in a single map

.map(id => ({
    ...previousState[id],
    ...payload.data,
}));

[fzaninotto]

I think the reduce has another purpose: avoid updating non-existing ids.

[ThieryMichel]

Even so, it makes no difference. We create a new array any way from the ids in the payload.
And we will always merge the previousState with the data from the payloads, so we might as well do it in a single map.

[djhi]

agreed

[fzaninotto]

It took me some time to understand the purpose of this PR so let me summarize my findings here.

The undo saga catches every UNDOABLE action and dispatches the inner action, modified as follows:

• the name is changed to '[original_name]_OPTIMISTIC'
• the optimistic: true meta is added.
• other modifications not relevant to this issue

For instance, for an undoable crudDelete action, the saga dispatches the following action:

// Original action
{
    type: CRUD_DELETE,
    payload: { id, previousData },
    meta: {
        resource,
        fetch: DELETE,
        onSuccess: {
            notification: {
                body: 'ra.notification.deleted',
                level: 'info',
                messageArgs: {
                    smart_count: 1,
                },
            },
            redirectTo,
            basePath,
        },
        onFailure: {
            notification: {
                body: 'ra.notification.http_error',
                level: 'warning',
            },
        },
    },
}

// dispatched action
{
    type: CRUD_DELETE_OPTIMISTIC,
    payload: { id, previousData },
    meta: {
        optimistic: true,
        resource,
        fetch: DELETE,
        notification: {
            body: 'ra.notification.deleted',
            level: 'info',
            messageArgs: {
                smart_count: 1,
            },
        },
        redirectTo,
        basePath,
    },
}

This is fine.

The problem lies in the data reducer, which catches optimistic actions to modify the store immediately (without waiting for the server response). And it catches them by name, i.e. it will react to CRUD_DELETE_OPTIMISTIC.

This prevents usage of custom optimistic actions relying on the same fetch metas, for instance the ACCEPT_CHANGE action described by @djhi. Once redispatched by the undo saga, this action will become ACCEPT_CHANGE_OPTIMISTIC, and will be ignored by the data reducer. This is a pity, since this action is basically a CRUD_UPDATE with a special name.

So the proposal is to change the data reducer to react not on the optimistic action names anymore, but on the optimistic meta instead. that way, the data reducer would react to the ACCEPT_CHANGE_OPTIMISTIC action, not because of its name, but because it's optimistic.

I'm 👍 for that change.

[fzaninotto]

I think the reduce has another purpose: avoid updating non-existing ids.