Added proper query serializer #75

AlexanderArvidsson · 2020-12-11T16:06:07Z

The current method of storing the query in the cache is a simple JSON.stringify and JSON.parse. This has some problems; you cannot serialize certain objects, most notably Dates. Querying for dates in firestore is impossible with the current method because it is converted into a string due to the JSON serializer.

This PR implements the base of a proper serializer, that will maintain dates in the query, allowing you to correctly query for dates in your database.
It also allows future objects to be serialized correctly, by simply adding specific logic to the serializer.

src/helpers/serializer.ts

AlexanderArvidsson · 2020-12-12T15:26:52Z

src/classes/Fuego.ts

@@ -11,6 +11,7 @@ export class Fuego {
  public auth: typeof firebase.auth
  public functions: typeof firebase.functions
  public storage: typeof firebase.storage
+  public DocumentReference = firebase.firestore.DocumentReference


I managed to fix compatibility with Firebase 8.x by placing the DocumentReference inside the Fuego class.
So, for those that use 8.x, all they need to do is add the DocumentReference to their Fuego instance.
It's not a breaking change if they don't add it, because people with a custom Fuego will still be able to use the library like usual, and their document references won't be serialized properly (which it didn't before anyway) until they add DocumentReference

Creating the document reference in my older comment is also done using fuego.db.doc instead now.
Does this seem like a good solution?

Doing this via the Fuego instance is ideal. It's nice to have an entry-point that manages all things compatibility. Does 8.x remove this type altogether or just rename it?

It doesn't remove it, but since imports are changed from import * as firebase from to import firebase from it means the version different messes things up. By providing the right firebase in Fuego, this doesn't happen.

I see Fuego as a very nice compatibility interface between different versions, and because we pass in the functions and storage that way, why not pass in firestore as well so we have access to the right variables.

src/hooks/use-swr-collection.ts

src/helpers/serializer.ts

nandorojo · 2020-12-12T15:44:55Z

src/types/Query.ts

+   */
+  endBefore?: number
+
+  // THESE ARE NOT JSON SERIALIZABLE


While we're at it, might be a good time to knock these out?

Definitely yes! I'll look into that right now.

Do you know if there is a way to construct a snapshot?

I've managed to get it working with snapshots. Instead of serializing and re-constructing the snapshot, I store it away in a map and then fetch it when deserializing.

I do not know how using multiple useCollection with the same snapshots will affect this. If it has problems, we could try changing the serializer from static to have one serializer per useCollection. Then, we can store the fuego inside the Serializer rather than passing it as a parameter, and we don't have to worry about other hooks interfering with the cache.

@nandorojo Before merging, we need to investigate this above. Will it be a problem?

src/types/Query.ts

src/helpers/serializer.ts

alistairholt · 2020-12-30T03:44:15Z

What's the status on this PR? I noticed we can't use document refs in where conditions right now. This looks like it would fix it.

nandorojo · 2020-12-30T03:46:38Z

Hey, I've been really busy with work so I haven't had time to finish reviewing and testing it out. I'll do my best to get to it soon.

alistairholt · 2020-12-30T03:51:49Z

@nandorojo that would be amazing. Thank you for the extremely quick response.

nandorojo · 2020-12-30T04:30:51Z

You got it

AlexanderArvidsson · 2020-12-30T13:47:46Z

As I said in one of my comments, we should investigate if the cache could cause problems when using multiple hooks at the same time before merging this.

I do not know how using multiple useCollection with the same snapshots will affect this. If it has problems, we could try changing the serializer from static to have one serializer per useCollection. Then, we can store the fuego inside the Serializer rather than passing it as a parameter, and we don't have to worry about other hooks interfering with the cache.

brunoalano · 2021-01-07T22:03:22Z

While this PR don't get merged, there are alternatives to query with ReferenceDocument?

nandorojo · 2020-12-13T16:46:12Z

src/helpers/serializer.ts

+  // Deserializer function for query
+  public static deserializeQuery<Data extends object = {}>(
+    queryString: string,
+    fuego: Fuego


This is technically a global variable, since it's set from the config step of this library. I wonder if we should just import that directly. I'll think about it.

The reason for passing the fuego is to ensure the same version of firebase is used, because there are differences in the typings. I had trouble using it with my project due to this.

That should still be solved at the initialization step, with something like this: #59 (comment)

I do use my own Fuego instance, but I ran into problems with DocumentReference not being available in different versions of firebase, or at least available in different locations. If you have any suggested changes to fix this then that is welcome, the sooner we can merge this the better.

nandorojo · 2021-01-12T05:58:33Z

@brunoalano at the moment, not that I know of

oliver-ni · 2021-05-09T10:14:18Z

Will this be merged?

nandorojo · 2021-05-09T10:30:49Z

src/helpers/serializer.ts

+    }
+    // Date: Inject serializer options if not specified
+    if (where[2] instanceof Date && !where[3]) {
+      return [...where.slice(0, 3), { type: 'date' }] as WhereItem<Doc>


Shouldn't this turn the date into a serializable value, such as milliseconds or a string?

Or I suppose JSON.stringify at the last step can handle Dates, never mind

nandorojo · 2021-05-09T14:53:52Z

I can give it a final look with comments I left

jckw · 2021-07-14T15:34:43Z

How can I help with this PR?

nandorojo · 2021-07-14T15:36:24Z

You could test all the changes with real data in your app, and give input to my open comments to help push it along

nandorojo#75

AlexanderArvidsson · 2021-07-19T21:45:40Z

How can I help with this PR?

To be honest, I do not see any reason to not merge this at this point. I've been using it in prod for a while.
The reason I guess that it has not been merged is because I have a slight worry that there may be cache conflicts. This can be solved quite easily as I have described before. However, I do not have time to do these myself unfortunately.

I do not know how using multiple useCollection with the same snapshots will affect this. If it has problems, we could try changing the serializer from static to have one serializer per useCollection. Then, we can store the fuego inside the Serializer rather than passing it as a parameter, and we don't have to worry about other hooks interfering with the cache.

However, this may only be a problem when serializing DocumentReference:

// THESE ARE NOT JSON SERIALIZABLE
// startAt?: number | DocumentSnapshot
// endAt?: number | DocumentSnapshot
// startAfter?: number | DocumentSnapshot
// endBefore?: number | DocumentSnapshot

jckw · 2021-08-05T10:00:17Z

Have you tried using listen: true with this branch? I think I'm having problems related to it when I use a where query (not necessarily using dates).

E.g. something like this from the examples doesn't seem to work

const { data } = useCollection('users', {
  where: ['name', '==', 'fernando'],
  limit: 10,
  orderBy: ['age', 'desc'],
  listen: true,
})

jckw · 2021-08-05T10:44:48Z

In fact, on further inspection, this looks like a more general problem with just using a where filter that uses the == operator.

Might be related to #77.

jckw · 2021-08-05T13:00:13Z

My bad. This looks like it was due to a missing index. The error for this isn't surfaced anywhere though, and would be passed as the second arg to this querySnapshot-handling function.

swr-firestore/src/hooks/use-swr-collection.ts

Line 99 in 6884464

const data: Doc[] = await ref.get().then(querySnapshot => {

Added proper query serializer

5a99d46

AlexanderArvidsson mentioned this pull request Dec 11, 2020

Bug with useCollection and where by timeStamp. #51

Open

AlexanderArvidsson added 3 commits December 11, 2020 17:07

Removed debug print

f331a5c

Some type additions

c46f6b2

Added test case for string serialization

b536907

nandorojo reviewed Dec 11, 2020

View reviewed changes

src/helpers/serializer.ts Outdated Show resolved Hide resolved

Added support for serializing DocumentReference

3ece903

AlexanderArvidsson commented Dec 12, 2020

View reviewed changes

src/helpers/serializer.ts Outdated Show resolved Hide resolved

AlexanderArvidsson commented Dec 12, 2020

View reviewed changes

src/helpers/serializer.ts Outdated Show resolved Hide resolved

AlexanderArvidsson added 2 commits December 12, 2020 16:25

Add compatibility with firebase 8.x

72d3e04

Fix possible undefined DocumentReference

68b7a53

AlexanderArvidsson commented Dec 12, 2020

View reviewed changes

Simplified SerializerOptions type

c28bcab

nandorojo reviewed Dec 12, 2020

View reviewed changes

src/hooks/use-swr-collection.ts Show resolved Hide resolved

nandorojo reviewed Dec 12, 2020

View reviewed changes

src/helpers/serializer.ts Outdated Show resolved Hide resolved

nandorojo reviewed Dec 12, 2020

View reviewed changes

AlexanderArvidsson added 2 commits December 12, 2020 17:47

Added experimental snapshot serialization

e7b8104

Added some undefined checks

419e65d

nandorojo reviewed Dec 13, 2020

View reviewed changes

src/types/Query.ts Outdated Show resolved Hide resolved

src/helpers/serializer.ts Outdated Show resolved Hide resolved

nandorojo reviewed Dec 13, 2020

View reviewed changes

src/helpers/serializer.ts Outdated Show resolved Hide resolved

Removed debug prints

cba7971

This comment has been minimized.

Sign in to view

nandorojo reviewed Jan 12, 2021

View reviewed changes

nandorojo mentioned this pull request Mar 6, 2021

Basic Paging - StartAfter error with snapshot and index #95

Closed

nandorojo reviewed May 9, 2021

View reviewed changes

nandorojo mentioned this pull request May 9, 2021

Help wanted #117

Open

4 tasks

jckw added a commit to Houston-App/swr-firestore that referenced this pull request Jul 14, 2021

Add query serializer from AlexanderArvidsson.

a1b844d

nandorojo#75

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added proper query serializer #75

Added proper query serializer #75

AlexanderArvidsson commented Dec 11, 2020 •

edited

Loading

AlexanderArvidsson Dec 12, 2020 •

edited

Loading

nandorojo Dec 12, 2020 •

edited

Loading

AlexanderArvidsson Dec 12, 2020 •

edited

Loading

nandorojo Dec 12, 2020

AlexanderArvidsson Dec 12, 2020

AlexanderArvidsson Dec 12, 2020

AlexanderArvidsson Dec 12, 2020 •

edited

Loading

AlexanderArvidsson Dec 14, 2020

alistairholt commented Dec 30, 2020

nandorojo commented Dec 30, 2020

alistairholt commented Dec 30, 2020

nandorojo commented Dec 30, 2020

AlexanderArvidsson commented Dec 30, 2020

This comment has been minimized.

brunoalano commented Jan 7, 2021

nandorojo Dec 13, 2020

AlexanderArvidsson Jan 12, 2021

nandorojo Jan 12, 2021

AlexanderArvidsson Jan 12, 2021

nandorojo commented Jan 12, 2021

oliver-ni commented May 9, 2021

nandorojo May 9, 2021

nandorojo May 9, 2021

nandorojo commented May 9, 2021

jckw commented Jul 14, 2021

nandorojo commented Jul 14, 2021

AlexanderArvidsson commented Jul 19, 2021 •

edited

Loading

jckw commented Aug 5, 2021

jckw commented Aug 5, 2021

jckw commented Aug 5, 2021

Added proper query serializer #75

Are you sure you want to change the base?

Added proper query serializer #75

Conversation

AlexanderArvidsson commented Dec 11, 2020 • edited Loading

AlexanderArvidsson Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

nandorojo Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

AlexanderArvidsson Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderArvidsson Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alistairholt commented Dec 30, 2020

nandorojo commented Dec 30, 2020

alistairholt commented Dec 30, 2020

nandorojo commented Dec 30, 2020

AlexanderArvidsson commented Dec 30, 2020

This comment has been minimized.

brunoalano commented Jan 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nandorojo commented Jan 12, 2021

oliver-ni commented May 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nandorojo commented May 9, 2021

jckw commented Jul 14, 2021

nandorojo commented Jul 14, 2021

AlexanderArvidsson commented Jul 19, 2021 • edited Loading

jckw commented Aug 5, 2021

jckw commented Aug 5, 2021

jckw commented Aug 5, 2021

AlexanderArvidsson commented Dec 11, 2020 •

edited

Loading

AlexanderArvidsson Dec 12, 2020 •

edited

Loading

nandorojo Dec 12, 2020 •

edited

Loading

AlexanderArvidsson Dec 12, 2020 •

edited

Loading

AlexanderArvidsson Dec 12, 2020 •

edited

Loading

AlexanderArvidsson commented Jul 19, 2021 •

edited

Loading