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 printed query for deduplication, cache print calls #10968

Merged
merged 5 commits into from
Jun 14, 2023

Conversation

phryneas
Copy link
Member

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

This change should probably not need any new tests.

@changeset-bot
Copy link

changeset-bot bot commented Jun 12, 2023

🦋 Changeset detected

Latest commit: 86bab47

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Jun 12, 2023

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 36.57 KB (+0.14% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 42.98 KB (+0.12% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 32.49 KB (+0.18% 🔺)
import { ApolloProvider } from "dist/react/index.js" 1.3 KB (0%)
import { useQuery } from "dist/react/index.js" 4.34 KB (0%)
import { useLazyQuery } from "dist/react/index.js" 4.66 KB (0%)
import { useMutation } from "dist/react/index.js" 2.57 KB (0%)
import { useSubscription } from "dist/react/index.js" 2.34 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" 3.45 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" 2.89 KB (0%)
import { useReadQuery } from "dist/react/index.js" 1.65 KB (0%)
import { useFragment } from "dist/react/index.js" 2.12 KB (0%)

import { canUseWeakMap } from "../common/canUse"

const printCache = canUseWeakMap ? new WeakMap() : undefined;
export const print: typeof print_orig = (ast) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking of using optimism here, but optimism doesn't use a WeakMap internally, so printed "temporary" queries might hang around much longer that way.

@phryneas phryneas self-assigned this Jun 12, 2023
Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

By chance, do you have an issue or reference on what prompted this change? I'd love to understand what this is fixing/improving.

Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding this file to .prettierignore and formatting this? Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do so tomorrow :)

@@ -0,0 +1,14 @@
import { print as print_orig } from "graphql"
Copy link
Member

Choose a reason for hiding this comment

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

Would we avoid mixing casing and use origPrint or printOrig as the variable name here?

.changeset/rude-frogs-destroy.md Outdated Show resolved Hide resolved
@phryneas
Copy link
Member Author

@jerelmiller it would save us from having to add something like apollographql/apollo-client-nextjs@305f0b7 to the @apollo/experimental-nextjs-app-support package.

There, we get the query streamed in from the server in a JSON-stringified DocumentNode and parse that. We want to simulate that query being active in inFlightLinkObservables, but of course in the browser it comes from the source code, so we are missing out on object identity.

Easiest way to support this on the client side would be to have the inFlightLinkObservables indexed by a printed query, not by a query instance (it's not a WeakMap anyways and will be cleaned up soon enough) - but that requires us to call print twice, at least - so at that point it seems like a good idea to start memoizing print.

Apart from being a change for the nextjs bundle, this will also benefit users who declare their query twice with equal contents (as those before had no query deduplication), and will cache print calls, which is generally a good idea if a query is called a lot.

phryneas and others added 2 commits June 13, 2023 15:09
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
@phryneas
Copy link
Member Author

@jerelmiller I applied your suggestions

@phryneas phryneas requested a review from jerelmiller June 13, 2023 13:10
Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

🎉 🔥 👍 💯

@jerelmiller jerelmiller merged commit b102390 into release-3.8 Jun 14, 2023
@jerelmiller jerelmiller deleted the pr/printed-dedup branch June 14, 2023 17:22
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants