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

Compare variables based on their serialized value #427

Open
gregmartyn opened this issue Feb 21, 2024 · 1 comment
Open

Compare variables based on their serialized value #427

gregmartyn opened this issue Feb 21, 2024 · 1 comment
Labels
core Feature requests related to core functionality

Comments

@gregmartyn
Copy link

Apollo Client currently uses @wry/equality to determine whether variables has changed between invocations of useQuery. See: source 1, source 2, source 3 and maybe more.

That works fine for the specific classes that equality has special handling for (e.g. Date) and standard objects and primitives, but doesn't work for custom classes used by custom scalars. Unlike standard objects, equality compares instances of custom classes by reference, not by value.

equality doesn't have a special-case for the Temporal classes used by graphql-temporal, so useQuery(query, { variables: { aDate: Temporal.Instant.from('2020-01-01T00:00+00') } }) results in an infinite loop of identical requests, all with aDate=2020-01-01T00:00:00Z.

I propose that variables should be compared by JSON.stringify()'ing them instead. !equal(newOptions.variables, oldVariables) would become JSON.stringify(newOptions.variables) !== JSON.stringify(oldVariables). All variables have to be stringified to go over the wire anyway, so this should be compatible with all types. This should be backward compatible because any code relying on useQuery checking reference equality would have infinite-looped.

@jerelmiller jerelmiller added the core Feature requests related to core functionality label Feb 21, 2024
@jerelmiller
Copy link
Member

Hey @gregmartyn 👋

Interesting idea! To be honest, I think in general we need a more holistic approach to custom scalars anyways (something there is a lot of interest in: #368). This might be a good test case for whatever solution we introduce for that feature to ensure we serialize variables before comparing them.

Thanks for bringing this case up! This is really good for us to be thinking about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Feature requests related to core functionality
Projects
None yet
Development

No branches or pull requests

2 participants