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

useCollection: new fetchOnMount option #1326

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

srosset81
Copy link
Contributor

Add a new fetchOnMount option to the useCollection hook.

If false (true by default), the collection will not be fetched on load but can be fetched later with the refetch

For the useInbox and useOutbox hooks, it is false by default because most of the usages of useOutbox is to post activities, and thus we don't need to fetch the outbox.

TODO

  • Ensure refetch works
  • Check that useOutbox and useInbox still work, notably with awaitActivity and liveUpdates
  • Add documentation

@Laurin-W
Copy link
Contributor

I was thinking of a use case for the exact opposite too:
Ask the hook to prefetch the first say 20 items. This might be useful for example, if the inbox pages have a small number of items (some items might be hidden due to ACL constraints).

We could name that prefetch property for example?


Ensure refetch works

Why refetch and not fetchNextPage?

@srosset81
Copy link
Contributor Author

We could name that prefetch property for example?

You suggest to set a prefetch option that defines the number of pages ? With a default to 1, and the possibility to set it to 0 or more than 1 ? In that case, pagesToPrefetch could be more explicit.

Why refetch and not fetchNextPage?

The react-query doc says to use refetch if we don't enable the automatic query. Not sure if fetchNextPage work. I was thinking about maybe returning it as fetch when we use the fetchOnMount: false option.

@Laurin-W
Copy link
Contributor

You suggest to set a prefetch option that defines the number of pages ?

I was rather thinking of prefetching a certain number of items so that we can ensure that a page is filled with a sufficient number of items?

Why refetch and not fetchNextPage?

The react-query doc says to use refetch if we don't enable the automatic query. Not sure if fetchNextPage work. I was thinking about maybe returning it as fetch when we use the fetchOnMount: false option.

I think this might be a bit unintuitive to have two different approaches, depending on how many items were preloaded?

@srosset81
Copy link
Contributor Author

I was rather thinking of prefetching a certain number of items so that we can ensure that a page is filled with a sufficient number of items?

I don't understand. This is the current default behaviour: every time we use useCollection, the first page is prefetched.

@Laurin-W
Copy link
Contributor

Sorry, I think I wasn't very clear :)
Since the client cannot control the size of the page, the pages are possibly small (say 5 items).
So by setting prefetch to 12, the first three pages would be fetched at the beginning without additional calls to fetchNextPage.

@srosset81
Copy link
Contributor Author

Indeed that seems useful. And if we don't want to prefetch any items, we can just set prefetch to 0.

I would expect prefetch to be a boolean though, so maybe prefetchItems or numItemsToPrefetch would be better.

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