Feat : withLifecycleCallbacks support for wilcard * and array of callbacks + refacto

[quentin-decre]

Problem

Using withLifecycleCallbacks when you have many callbacks can be very verbose and hard to read / maintain. This can be improved a lot with two features :

1. Support for a wildcard resource : *

Imagine for example your API returns "identifier" and you want to rename it to "id" to make it work with react-admin. You will need to create a callback for each resource. But now you can do that :

const dataProvider = withLifecycleCallbacks(
  jsonServerProvider('http://localhost:3000'), [
  {
    resource: '*',
    afterRead: async (data, { dataProvider, resource }) => {
      // rename field to the record
      data.id = data.identifier
      return data
    },
  },
])

2. Support for arrays of callbacks

When you have a lot of callbacks / hooks (which is often the case when having a graphql API), you may want to have one callback per purpose, and name it after that purpose. So you could have many callbacks for the same resource / hook. This is already possible but quite verbose / difficult to read.
The idea is to allow to pass an array of callbacks, while still supporting passing directly one callback.

Currently :

      {
        resource: '*',
        beforeGetList: defaultSortById,
      },
      {
        resource: '*',
        beforeGetList: addNullsLastToSortOrders,
      },
      {
        resource: '*',
        beforeGetList: convertFilters,
      },

After :

      {
        resource: '*',
        beforeGetList: [defaultSortById, addNullsLastToSortOrders, convertFilters],
      },

[djhi]

This looks great 👍 I like that it encourages good practices and better readability. Would you mind adding some tests and updating the documentation?

[fzaninotto]

As this is a new feature, it must be PRed against next rather than master

[fzaninotto]

This is a breaking change. You can add the same feature in a backward compatible way, by adding a third parameter instead.

[quentin-decre]

changed

[fzaninotto]

This is another breaking change: beforeSave used to be applied on params.data, not on params

[quentin-decre]

changed

[fzaninotto]

same

[quentin-decre]

changed

[fzaninotto]

You should create a type for this union, to avoid repetition

[quentin-decre]

changed

[fzaninotto]

I suggest you extract that function, and unit test it.

[quentin-decre]

If I extract this function, I see two options :

1. either we have to pass the handlers + dataProvider each time, the drawback is that it's quite verbose and the code is less clear :

newParams = await applyCallbacks(
                dataProvider,
                handlers,
                resource,
                'beforeGetList',
                newParams
            );

2. Or I could bind the the repeated parameters :

    const applyCallbacksWithContext = applyCallbacks.bind(
        null,
        dataProvider,
        handlers
    );

....

newParams = await applyCallbacksWithContext(
                resource,
                'beforeGetList',
                newParams
            );

I tend to prefer the first option because the code is less complex to understand even though it's very verbose.

What is your opinion on that ?

[fzaninotto]

Why do you export it?

[quentin-decre]

I use it in my own code to apply a callback for every possible hook (for login purpose essentially).
I understand this could be in my own code though.

[quentin-decre]

Regarding the documentation, I hesitated between added the new features in the main part of the documentation, or add an "advanced use" section, and explain the new possibilities, and how they are very useful, in which cases, best practices, ....
Also add some details about execution order and the possibility to add the same resource multiple times (it was already possible, but not very clear for me in the documentation).
What is your opinion on that ?

Can you tell me if the implementation is OK for you ? If yes, I will move on unit testing :)

[djhi]

Thanks!

Regarding the documentation, I hesitated between added the new features in the main part of the documentation, or add an "advanced use" section, and explain the new possibilities, and how they are very useful, in which cases, best practices, ....

I would probably add this in the Code Organization section.

Also add some details about execution order and the possibility to add the same resource multiple times (it was already possible, but not very clear for me in the documentation).

This is definitely worth adding in the Usage section

[djhi]

As this is not used anywhere, I believe we should remove it

[quentin-decre]

I have fixed + added some unit tests for the new features

[quentin-decre]

Thanks!

Regarding the documentation, I hesitated between added the new features in the main part of the documentation, or add an "advanced use" section, and explain the new possibilities, and how they are very useful, in which cases, best practices, ....

I would probably add this in the Code Organization section.

Also add some details about execution order and the possibility to add the same resource multiple times (it was already possible, but not very clear for me in the documentation).

This is definitely worth adding in the Usage section

agreed, did that :)

[djhi]

Can you please remove the package-lock.json file? Besides, I don't get why there are changes in the yarn.lock file.

[quentin-decre]

Can you please remove the package-lock.json file? Besides, I don't get why there are changes in the yarn.lock file.

deleted + reverted :)

[fzaninotto]

Thanks!