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

Add a GET_COUNT fetch action for graphql dataProvider #5403

Closed

Conversation

EmrysMyrddin
Copy link
Contributor

@EmrysMyrddin EmrysMyrddin commented Oct 14, 2020

In the graphql provider, we have to define a query to fetch the total count of entity for a given resource. Previously, the name of this query was hardcoded.

This PR aims to allow the customization of this query name, such as every other query names.

I'm not sure about where to put the new GET_COUNT constant, tell me if you think there is a better place.

I have exported GET_COUNT from ra-data-graphql-simple to allow an easier use for developers, allowing to import this simbol directly from ra-data-graphql-simple instead of having an explicit dependence on ra-data-graphql in the application code.

Closes #4566

@EmrysMyrddin
Copy link
Contributor Author

I have a bug, it doesn't work if you don't provide a custom GET_COUNT operation name. Working on it and adding some errors with more useful informations for the developer.

@EmrysMyrddin
Copy link
Contributor Author

EmrysMyrddin commented Oct 15, 2020

I have updated the unit tests but it shows that we potentially have a breaking change with this feature on the ra-data-graphql package.

ra-data-graphql-simple implicitly relies on a _allResourcesMeta query, but it was not the case of ra-data-graphql. This PR explicitly defines a GET_COUNT operation in ra-data-graphql, so if someone was using ra-data-graphql instead of ra-data-graphql-simple for some reason, it can break if they don't have this query.

I don't know if this is fine or not. If it's not, do you have some inputs about how to implement this without breaking change ?

A quick fix could be to remove the check in isResource. This way, any existing resource without the GET_COUNT operation should be ok, but this means the error report in case of missing GET_COUNT query when using ra-data-graphql-simple will be less explicit and thrown only when a GET_LIST or GET_MANY fetch is actually executed.

@EmrysMyrddin EmrysMyrddin force-pushed the get-count-custom-query-name branch 2 times, most recently from e1c1c07 to 954a5f1 Compare October 15, 2020 09:38
@EmrysMyrddin
Copy link
Contributor Author

After more thought, there is no explicit error message in the isResource, so it bring nothing to make GET_COUNT operation required in ra-data-graphql.

There is no more breaking change. I have just added a more explicit error in ra-data-graphql-simple for missing GET_COUNT operation, which was implicitly required before.

@EmrysMyrddin
Copy link
Contributor Author

EmrysMyrddin commented Oct 16, 2020

I have also fixed a bug where the params where not filtered out if GET_COUNT query didn't declared them. The #2711 will be closable if this is merged :-)

@EmrysMyrddin
Copy link
Contributor Author

Just a little update, Is there something I should do to push this PR forward ? We would like to have this feature before deploying our backoffice based on react-admin to production :-)

@EmrysMyrddin
Copy link
Contributor Author

@fzaninotto @djhi Just a friendly ping :-) Do I have something to do for this PR to be merged ?

@patou
Copy link

patou commented Aug 12, 2021

Any chance to view this PR merged ?

@mael-morel
Copy link

Hi, is it possible to merge this PR ?

@fzaninotto
Copy link
Member

it needs rebase, can only be pushed to next, and will have to wait until we overhaul the current GraphQL provider.

@mael-morel
Copy link

ok, thanks for the reply

@fzaninotto fzaninotto changed the base branch from master to 3.x April 12, 2022 16:45
@fzaninotto fzaninotto added the v3 label Apr 12, 2022
@fzaninotto
Copy link
Member

React-admin version 3 is feature freeze. Can you please rebase this PR against next, and make it compatible with v4?

@EmrysMyrddin
Copy link
Contributor Author

I'm closing this PR. I don't use this anymore and I'm not sure if it's useful to spare time on a rebase.

Don't hesitate to comment if you think that it could be interesting to work on it, I could look at it if you want to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the GET_COUNT data fetch action
4 participants