[RFC] Replace Providers by objects

[fzaninotto]

Problems

It's impossible to give useful Typescript types to the dataProvider, authProvider, and (since #3685) i18nProvider because they are functions accepting a type parameter as first argument, and with a return value depending on that type. This makes writing custom providers a bit harder.

Besides, the switch/case syntax of providers is sometimes seen as a bad coding practice.

Lastly, all the constants we use (like GET_ONE, AUTH_LOGOUT, etc) make the code heavier and longer to write.

Proposal

React-admin calls the Provider functions with a message type and arguments. There is another way to do that, more idiomatic to JavaScript: calling methods on an object.

I propose to transform the syntax of all 3 providers so that they can be objects.

For instance, an authProvider written as a function:

import { AUTH_LOGIN, AUTH_LOGOUT, AUTH_CHECK, AUTH_ERROR } from 'react-admin';

export default (type, params) => {
    if (type === AUTH_LOGIN) {
        const { username } = params;
        localStorage.setItem('username', username);
        // accept all username/password combinations
        return Promise.resolve();
    }
    if (type === AUTH_LOGOUT) {
        localStorage.removeItem('username');
        return Promise.resolve();
    }
    if (type === AUTH_ERROR) {
        return Promise.resolve();
    }
    if (type === AUTH_CHECK) {
        return localStorage.getItem('username')
            ? Promise.resolve()
            : Promise.reject();
    }
    return Promise.reject('Unkown method');
};

Can be written as an object as follows:

export default {
    login({ username }) {
        localStorage.setItem('username', username);
        // accept all username/password combinations
        return Promise.resolve();
    },
    logout() {
        localStorage.removeItem('username');
        return Promise.resolve();
    },
    error() {
        return Promise.resolve();
    },
    check() {
        return localStorage.getItem('username')
            ? Promise.resolve()
            : Promise.reject();
    }
}

This is very easy to Type using TypeScript. Something like:

type AuthProvider {
    login: (args: any) => Promise<void>;
    logout: () => Promise<void>;
    error: (error: any) => Promise<void>;
    check: () => Promise<void>
}

Backward Compatibility

This is not a breaking change if react-admin detects that a provider is a function rather than an object, and wraps it inside a converter function:

import { AUTH_LOGIN, AUTH_LOGOUT, AUTH_ERROR, AUTH_CHECK } from 'react-admin';

const convertFunctionAuthProvider = authProvider => ({
    login: args => authProvider(AUTH_LOGIN, args),
    logout: () => authProvider(AUTH_LOGOUT),
    error: error => authProvider(AUTH_LOGOUT, error),
    check: ()=> authProvider(AUTH_CHECK)
})

So we can make it painless for the users, and preserve the large list of available (data-) providers.

Notes

We've been using composition as an argument for using functions for providers. For instance, to use two different providers for two sets of resources:

const userProvider = simpleRestProvider('http://my.domain.com/users'); // (type, resource, payload => Promise
const postProvider = simpleRestProvider('http://my.other.domain.com/posts'); // (type, resource, payload => Promise

const dataProvider = (type, resource, payload) => {
    if (resource === 'users') return userProvider(type, resource, payload);
    if (resource === 'posts') return postProvider(type, resource, payload);
}

This is very practical, but nothing you can't do with objects:

type DataProvider {
    getList: (resource: string, params: any) => Promise<Record[]>;
    getOne: (resource: string, id: Identifier) => Promise<Record>;
   // ... 
}

const userProvider = simpleRestProvider('http://my.domain.com/users'); // DataProvider
const postProvider = simpleRestProvider('http://my.other.domain.com/posts'); // DataProvider

const dataProvider = {
   __noSuchMethod__: (id, args) => {
        if (args[1] === 'users') return userProvider[id].apply(userProvider, args),
        if (args[1] === 'posts') return postProvider[id].apply(postProvider, args),
    }
}

What's your take?

I may forget some pros and cons, please comment on this issue to give your opinion!

[fzaninotto]

I forgot #3340: because they are functions, our providers aren't really practical to use in a useState() call... Turning them to objects would remove that gotcha (that I encountered myself).

[fzaninotto]

A con I didn't think of: there is an upgrade cost for users, not when injecting providers, but when using them.

We can make it so there is no cost when using dataProvider through <Query> or <Mutation>:

import { Query } from 'react-admin';

const UserProfile = ({ record }) => (
    <Query type="GET_ONE" resource="users" payload={{ id: record.id }}>
        {({ data, loading, error }) => {
            if (loading) { return <Loading />; }
            if (error) { return <p>ERROR</p>; }
            return <div>User {data.username}</div>;
        }}
    </Query>
);

To make that work, we'll need to support both the GET_ONE and the getOne types. Not a big deal, and zero work for the end user.

But users will have to make changes to their code using withDataProvider:

// in src/comments/ApproveButton.js
import {
   showNotification,
   UPDATE,
   withDataProvider,
} from 'react-admin';

class ApproveButton extends Component {
    handleClick = () => {
        const { dataProvider, dispatch, record } = this.props;
        const updatedRecord = { ...record, is_approved: true };
-       dataProvider(UPDATE, 'comments', { id: record.id, data: updatedRecord })
+       dataProvider.update('comments', { id: record.id, data: updatedRecord })
            .then(() => {
                dispatch(showNotification('Comment approved'));
                dispatch(push('/comments'));
            })
            .catch((e) => {
                dispatch(showNotification('Error: comment not approved', 'warning'))
            });
    }

    render() {
        return <Button label="Approve" onClick={this.handleClick} />;
    }
}

ApproveButton.propTypes = {
    dataProvider: PropTypes.func.isRequired,
    dispatch: PropTypes.func.isRequired,
    record: PropTypes.object,
};

export default withDataProvider(ApproveButton)

I'm afraid that the cost of migration reduces the benefit/cost ratio.

That, and the fact that starting such a refactoring will delay the release of v3 by at least a couple weeks.

[fzaninotto]

For people who don't use TypeScript (and that's 100% of people since we don't publish types yet), this is just cost with no benefit... So I tend to think this is premature. We should only make that change when react-admin types are published, which will not be in v3.0 (too much work).

[fzaninotto]

I had a revelation while jogging in the woods with my dog: This change can be made in a totally backward-compatible way, because in JavaScript, functions are also objects.

So if developers pass a function provider to Admin, instead of replacing that function by an object provider, we can dynamically add methods to the function, calling the function itself. Something like:

const convertFunctionAuthProvider = authProvider => {
    authProvider.login = args => authProvider(AUTH_LOGIN, args);
    authProvider.logout = () => authProvider(AUTH_LOGOUT);
    authProvider.error = error => authProvider(AUTH_LOGOUT, error);
    authProvider.check = ()=> authProvider(AUTH_CHECK);
    return authProvider;
};

That way, both the internal react-admin calls (using methods like login) and legacy code by the developers (using function call with constant verbs) will work.

This change is therefore a net benefit with no cost for the developers. I'm +1 for incorporating it in v3.

[fzaninotto]

We've decided to do it, so I'm closing this issue.