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 totalCount field to plural connection type #954

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jaydenwindle
Copy link

@jaydenwindle jaydenwindle commented Jun 29, 2024

This PR adds an additional field totalCount to the connection type for all plural queries. If the totalCount field is included in a query, an additional database call is made to fetch the total number of unique records matching the currently applied where filters. If the totalCount field is not queried, no additional database calls are made.

@0xOlias
Copy link
Collaborator

0xOlias commented Jul 1, 2024

Hi, thanks for opening. I'm open to this addition, but have some ideas for how to tweak it:

query {
  friends(where: { id_lt: 100 }) {
    items {
      id
      name
      age
    }
    pageInfo {
      endCursor
      hasNextPage
    }
    totalCount # On the Page type, not the PageInfo type
  }
}
  • I'm mildly concerned that users will include totalCount more often than needed, which can easily wreck performance. When rendering a paginated view, you likely only need to query the total count once up-front. Don't have an immediate solution - might be as simple as a warning callout in the docs.
  • There's a bit more work to do to update the findMany function signature type, and I have some naming ideas there. Will follow up with a few commits. We also need to update two docs pages accordingly:

@jaydenwindle
Copy link
Author

Thanks for considering this!

We can definitely move totalCount into the Connection type, that makes a lot more sense.

I agree that the performance considerations should be mentioned in the docs. It seems like there may be a way to use CTEs to include both queries in a single database call without too much performance overhead. Happy to investigate this if performance is a big concern.

@jaydenwindle jaydenwindle changed the title Add totalCount field to pageInfo Add totalCount field to connection type Jul 4, 2024
@jaydenwindle
Copy link
Author

jaydenwindle commented Jul 4, 2024

@0xOlias I have refactored the totalCount query logic to use CTEs instead of a second database call in order to mitigate the performance concerns. I've also moved the totalCount field to the connection type, renamed the findMany argument to withTotalCount, and updated the documentation (with a warning about performance implications).

Let me know if there are any other changes you would like me to make!

@jaydenwindle jaydenwindle changed the title Add totalCount field to connection type Add totalCount field to plural connection type Jul 4, 2024
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.

2 participants