Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] add useOptimisticQuery hook #3248

Closed
wants to merge 2 commits into from
Closed

[WIP] add useOptimisticQuery hook #3248

wants to merge 2 commits into from

Conversation

fzaninotto
Copy link
Member

Allows to group two pieces of controller logic:

  • fetch the dataProvider using a Redux action
  • get the result from the store

I also introduced a new reducer to store the loading state of every request, which allows to pass that information to the view.

I updated the Edit and Show controllers to use that new hook. The isLoading prop is now relative to the main query (not the global loading state).

That hook fetches the data provider and returns the data from the a=cache as soon as it finds it.
): CrudGetOneAction => ({
type: CRUD_GET_ONE,
payload: { id },
meta: {
resource,
fetch: GET_ONE,
basePath,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

decoupling side effects, refs #2952

const { data: record, loading } = useGetOne(
resource,
id,
{
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now it's the controller who decides the side effects, not the action creator

* @returns {GetOneResponse} An object with the shape { data, error, loading, loaded }
*/
const useOptimisticQuery = (
action: FetchAction,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so sure about the signature of this one. Should it accept:

  • an action returned by an action creator, as I did here, or
  • a quadruplet { type, resource, payload, meta } , like the useQuery action

In the latter case, we don't need the crud action creators anymore.

@fzaninotto
Copy link
Member Author

After a good night's sleep, and the discernment of a week day, I think this experimentation is useful but does not show the best solution.

  1. Using the Redux store to store loading state of requests is an heresy. No component will need to access that state, so it better be local. We've already found a nice way to manage the loading state with the useDataProvider hook, so let's use this one.
  2. Action creators, when removing side effects, are all the same. They just populate the payload with their arguments, and craft the action type based on the crud verb. So they are useless - we could replace them all by a single function. As a consequence, we shouldn't use them, and instead promote the hook alternative (useGetOne, useGetList, etc), which actually return the content. We don't need to remove them from the core (that would be another breaking change), but we should deprecate them and remove them in a future major version.
  3. useQuery could benefit from the optimistic approach, too. It just needs a selector function to get the result from the store, and a custom reducer to store all CUSTOM_FETCH response payloads somewhere in the store.
  4. So there wouldn't be much difference between useQuery and useOptimisticQuery. We could merge the two implementations. A non-optimistic query would just be called with an empty selector.
  5. If we're using a useQuery-like API in our controllers, we have the benefit of a Promise API, so declarative side effects in the metas don't make much sense. I think the controllers should just dispatch the success and failure side effects themselves. That way, there would be only one way to dispatch side effects, and we could get rid of a lot of sagas. Also in a future major version.
  6. A useQuery accepting an action name and a selector starts having too many arguments, so we must change its signature, to something like:
useQuery(query, options);

//example
const useGetOne = (resource, id, sideEffects) => useQuery(
  { type: GET_ONE, resource: 'posts', payload: ' { id: 123 } },
  { action: CRUD_GET_ONE, selector: state => state.admin.resources[resource].data[id], meta: sideEffects }

It's not completely similar to the dataProvider signature anymore, but I think it's for the better.

So I'm going to close this PR and start working on a better implementation.

(note for self: do not work on weekends)

@fzaninotto fzaninotto closed this May 20, 2019
@fzaninotto fzaninotto mentioned this pull request May 20, 2019
10 tasks
@fzaninotto fzaninotto deleted the useGetOne branch July 19, 2019 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant