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

Use native sorting, filtering and paging, allow get query #5

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

gstvg
Copy link

@gstvg gstvg commented Oct 28, 2019

Use firestore's native sorting - orderBy(field, order) - filtering - where(field, op, value) - and paging - limit(limit) and offset(offset).

Allow returning query for use with ra-realtime saga when params.snapshot is true (or should params.snapshot be the callback for query.onSnapshot(callback) ?)

Didn't tested yet, will do soon.

Use firestore's native sorting - orderBy(field, order) - filtering - where(field, op, value) - and paging - limit(limit) and offset(offset).
Allow returning query for use with ra-realtime saga.
@gstvg gstvg mentioned this pull request Oct 28, 2019
package.json Outdated
@@ -96,6 +96,6 @@
"react": "16.9.0",
"react-admin": "^2.9.6",
"react-dom": "^16.9.0",
"sort-by": "^1.2.0"
"ra-realtime": "^2.8.6"
Copy link
Owner

Choose a reason for hiding this comment

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

Cool! Did not know this exists!

@gstvg
Copy link
Author

gstvg commented Oct 30, 2019

Native sorting and filtering worked fine, realtime support too, but it also updates unedited fields while editing, keeping intact modified fields. I didn't figured out a way to determine if a GET_ONE is for showing or editing. Need your opinion if this is an acceptable/desired behavior, if not, we can apply realtime updates only to GET_LIST.

Now the real issues. Native pagination doesn't work for two reasons:

  1. Firestore Web SDK doesn't expose Query.offset(), only limit(). At first i used only limit(page * perPage), then sliced the result, e.g: with perPage 10, page 1 would only fetch 10 items, page 2, 20, and so on. Better than fetching all resources in every page, but...
  2. Firestore doesn't have counting operation. So we need to fetch all items on every page to know how many items and next pages, if any, exists.

Possible partial solutions are:

For 1, a HTTP cloud function, using Node.js SDK, which exposes .offset(), which could return the page's docs directly, but then not supporting realtime, or returning first and last doc of the page, and then on client side querying with startAt and endAt. But again, no couting. So the cloud function could fetch all items, return either page docs or first and last, and the count, but it also defeats realtime support, since the observer expect the total count on new data, and the observed query with startAt and endAt would return at most perPage items. This could be solved with either a new cloud function for couting on realtime updates, or caching somewhere on client side, but it feels a bit hackish for me. Caching on client side first and last docs of last query also passed my mind, but it would allow only next and previus page, not allowing jumping more than one page nor opening a link with page bigger than one, and again, no counting...

For 2, keeping a total unfiltered count for a collection using firestore onCreate and onDelete hooks. But unfiltered and filtered GET_LIST's count would be the same, leaving filtered lists with 'ghost' pages on the end. I think that this isn't viable.

Let's not forget that so much reading costs money, and skipped docs with offset are billed too.

With all that in mind, probably pagination should keep working in the way it works today: fetch all docs, making firestore a data provider suitable only for systems with little data - or in my case - little data per tenant.

Soon i will deploy an online demo version with realtime support.

@gstvg
Copy link
Author

gstvg commented Oct 30, 2019

To relieve pressure, we could also fetch only the sorted field with query.select(sortedField), and then a full query with startAt and endAt. Things would get faster, but this would increase a bit billed reads.

@gstvg
Copy link
Author

gstvg commented Oct 30, 2019

We could implement a next&previous only pagination, as i saw here https://github.com/abiglobalhealth/aor-dynamodb-client. For react-admin, this would be like this https://marmelab.com/react-admin/List.html#pagination.

Probably we should allow both options to users of this lib: full pagination with fetch all docs for little data, and next&previous only fetching only page's docs for medium-to-big data. I think that relying on cloud functions adds too much complexity, and one of the reasons for me using RA+Firestore was overall simplicity, and it only partially solves the problem, while increasing costs too.

@rafalzawadzki
Copy link
Owner

@gstvg that's an impressive piece of research. I stumbled upon some of the limitations you mentioned myself, but due to limited time was never able to address them. Thank you!

Let me reply to you in points:
1) Realtime support for GET_ONE
I only had a brief look at RA documentation and indeed seems there is no differentiation between show/edit for GET_ONE.

I agree that realtime updates while editing would result in weird user experience. I'd keep it either as an optional feature (enabled explicitly as a feature flag in DataProvider config) or disable altogether. Looked up ra-firebase - seems they also do realtime updates only for lists.

2) Pagination
No .offset() or .count() provided by Firebase is a true bummer.

I'd avoid overcomplication with Cloud Functions and try to keep it simple with what Firestore provides. Thus:

  1. We will not spread ourselves thin
  2. We can enable developers to use this library as plug-and-play (as much as possible)
  3. We will not make the users incur Cloud Functions costs by accident

Seems to me like prev/next for pagination has a pretty good ROI, at least for now :)

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.

None yet

2 participants