connectrpc / connect-query-es

TypeScript-first expansion pack for TanStack Query that gives you Protobuf superpowers.
https://connectrpc.com/docs/web/query/getting-started
Apache License 2.0
237 stars 17 forks source link

Handle structural sharing #405

Closed smaye81 closed 1 month ago

smaye81 commented 1 month ago

Tanstack seems to have added a noisy console.error log for queries that return data that is not JSON-serializable. Since some of connect-query's tests use BigInt, we now see the following in the logs when running tests:

StructuralSharing requires data to be JSON serializable. To fix this, turn off structuralSharing or return JSON-serializable data from your queryFn. [@"connectrpc.eliza.v1.PaginatedService","List",#page:0,,"infinite",]: TypeError: Do not know how to serialize a BigInt

An attempt at disabling structuralSharing for tests was made in #404 but closed in favor of a proper investigation into handling this more robustly. Tanstack recommends to either turn off structuralSharing in your queries or to return JSON serializable data from your queryFn. So, we should investigate these as a better course of action.

paul-sachs commented 1 month ago

Actually, looking at the docs for useQuery, the structuralSharing option can also take the form of a function. If we just provide this option by modifying createUseQueryOptions that should actually handle this in all cases (we'll need to modify createUseInfiniteQueryOptions as well.)

Looking at the signature this might not even really require waiting for protobuf-es@v2 either since it's just a simple compare method that returns a bool.

timostamm commented 1 month ago

Looking at the signature this might not even really require waiting for protobuf-es@v2 either since it's just a simple compare method that returns a bool.

It's either a boolean, or a function that implements the structural sharing logic:

    structuralSharing?: boolean | ((oldData: unknown | undefined, newData: unknown) => unknown);

But I expect that a simplistic implementation can use protobuf-es' equals() to compare old with new, and return old if equal, maintaining the reference.

With v2, this needs to be a per-query option since equals() is a function that takes a schema as an argument instead of an object method with the schema curried in. Any change we make is ideally forwards-compatible.

I don't want to move too many things around at once, but I agree we don't technically have to wait for v2.

timostamm commented 1 month ago

This will be fixed in v2 by https://github.com/connectrpc/connect-query-es/pull/414. We will cut a pre-release shortly.