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

Under certain conditions a different key is generated for the same DocumentNode #1508

Closed
zenflow opened this issue Apr 3, 2021 · 1 comment · Fixed by #1509
Closed

Under certain conditions a different key is generated for the same DocumentNode #1508

zenflow opened this issue Apr 3, 2021 · 1 comment · Fixed by #1509
Labels
bug 🐛 Oh no! A bug or unintented behaviour.

Comments

@zenflow
Copy link
Contributor

zenflow commented Apr 3, 2021

Under certain conditions (all of which are a bit confusing) a different key is generated (by keyDocument()) given the same DocumentNode.

This is noticeable in a next-urql app when we see unexpected cache misses during either final server-side render (Error: ReactDOMServer does not yet support Suspense.) or client-side rehydration (since cached data isn't found, React warns about unexpected dom nodes, and data is fetched over network again).

versions are the latest as of writing this:

  • urql @ v2.0.2
  • next-urql @ v3.0.1

Uses next-urql's default exchanges

Steps to reproduce

  1. Clone & install the reproduction project: https://github.com/zenflow/urql-documentnode-key-generation-issue
  2. Start dev server with yarn next dev
  3. Open http://localhost:3000 & open Chrome devtools
  4. touch pages/index.tsx, or make a trivial change to it's contents like <h1> to <h2>
  5. Wait for recompile & then refresh the browser
  6. Observe error rendering server-side: Error: ReactDOMServer does not yet support Suspense.
  7. Refresh browser (or allow it to automatically refresh) until a page loads without the above error
  8. Observe console warning & network request to graphql server

Analysis

To debug I built packages/core with hash() modified to console.log it's input (unique graphql query string) & output (hash number).

I found that, for my single query, either of two different query strings would be hashed: "query MyQuery { ... }" (bare) or "# MyQuery\nquery MyQuery { ... }" (prefixed), resulting in a different hash number.

See in stringifyDocument() where the query string gets prefixed.

I see a problem that stringifyDocument() is not idempotent. When first called with a DocumentNode that has no .loc property, it adds the .loc property and returns only the bare query string. The next time it's called with the exact same DocumentNode object, since the .loc property is now present, the result will be prefixed with operation name if available.

TBH It was pretty confusing, because the code looks ok at first glance - you would think stringifyDocument() wouldn't be called twice with the same object anyway, due to short-circuiting in keyDocument() when that object has a .__key property defined.
But I looked closer and noticed that this .__key property is only added to the document objects that are being kept in the docs Map. Document objects that never get added to the docs Map (because another identical document object was "keyed" first) never have their .__key property defined, and are therefore stringified & hashed on each call to keyDocument().

@zenflow zenflow added the bug 🐛 Oh no! A bug or unintented behaviour. label Apr 3, 2021
@zenflow
Copy link
Contributor Author

zenflow commented Apr 3, 2021

BTW I ran into this problem because graphql-code-generator doesn't provide a .loc property on the TypedDocumentNodes it produces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Oh no! A bug or unintented behaviour.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant