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

Only dedupe pagination queries if the variables match #1378

Merged
merged 11 commits into from
Nov 22, 2024

Conversation

AlecAivazis
Copy link
Collaborator

This PR adds a new argument to the @dedupe directive: match which takes 3 values: Variables, Operation, and None. This gives the user more fine-grained control over how the deduping happens. Most importantly, for paginated queries, @dedupe comes with match set to Variables.

To help everyone out, please make sure your PR does the following:

  • Update the first line to point to the ticket that this PR fixes
  • Add a message that clearly describes the fix
  • If applicable, add a test that would fail without this fix
  • Make sure the unit and integration tests pass locally with pnpm run tests and cd integration && pnpm run tests
  • Includes a changeset if your fix affects the user with pnpm changeset

Copy link

changeset-bot bot commented Oct 24, 2024

🦋 Changeset detected

Latest commit: d401396

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

This PR includes changesets to release 8 packages
Name Type
houdini Patch
houdini-adapter-auto Patch
houdini-adapter-cloudflare Patch
houdini-adapter-node Patch
houdini-adapter-static Patch
houdini-react Patch
houdini-svelte Patch
houdini-plugin-svelte-global-stores 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

Copy link

netlify bot commented Oct 24, 2024

Deploy Preview for houdini-docs-next ready!

Name Link
🔨 Latest commit 573fc7f
🔍 Latest deploy log https://app.netlify.com/sites/houdini-docs-next/deploys/6740c8e10ab6fe000889e460
😎 Deploy Preview https://deploy-preview-1378--houdini-docs-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Oct 24, 2024

Deploy Preview for houdinigraphql ready!

Name Link
🔨 Latest commit 573fc7f
🔍 Latest deploy log https://app.netlify.com/sites/houdinigraphql/deploys/6740c8e197f17b000809c162
😎 Deploy Preview https://deploy-preview-1378--houdinigraphql.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@SeppahBaws SeppahBaws left a comment

Choose a reason for hiding this comment

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

Hmm this doesn't seem to be doing any deduping at all, regardless of what params I give it...

I'll try to add a small repro in the e2e over the weekend

@half2me
Copy link
Contributor

half2me commented Nov 2, 2024

Can we also either have the option to opt in/out of dedupe behaviour? Or at least disable it for SSR?
Currently all my tests are failing when running in parallel mode, because when multiple requests hit my endpoint that uses a @loading directive it throws a 500 with Aborted.

@AlecAivazis
Copy link
Collaborator Author

@half2me it should only be automatically deduping pagination queries. are you seeing something different?

@half2me
Copy link
Contributor

half2me commented Nov 4, 2024

@half2me it should only be automatically deduping pagination queries. are you seeing something different?

Yes I am. Since I upgraded to this, a lot of tests started failing with the server returning status 500 Aborted error, for any page where I'm using pagination.

This definitely has something to do with multiple requests hitting the same page, since if I disable parallel workers for my tests, and run them just one at a time, they all work fine. Its only when simultaneous requests hit the same page that they break.,

@AlecAivazis
Copy link
Collaborator Author

@SeppahBaws i think i fixed what was wrong

@half2me i added a new config value supressPaginationDeduplication that you can set to prevent the deduplication

@half2me
Copy link
Contributor

half2me commented Nov 5, 2024

Amazing, I will rerun the tests and let you know. @AlecAivazis is there an easy way I can pin the houdini package in my package.json to point to this branch?

@SeppahBaws
Copy link
Collaborator

Ah awesome, I'll give this a shot once it's calmed down a bit more at work...

@half2me you can clone and build the repo locally, and then link it locally to your repo with npm/pnpm link:
https://docs.npmjs.com/cli/v6/commands/npm-link
https://pnpm.io/cli/link

(on npm you can also specify the package version as "file:../path/to/houdini/packages/houdini" - I did find that this messes up in pnpm though)

SeppahBaws
SeppahBaws previously approved these changes Nov 22, 2024
Copy link
Collaborator

@SeppahBaws SeppahBaws left a comment

Choose a reason for hiding this comment

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

Looking good!

@AlecAivazis AlecAivazis merged commit d2dbcd2 into main Nov 22, 2024
15 checks passed
@AlecAivazis AlecAivazis deleted the dedupe-match-arg branch November 22, 2024 22:34
This was referenced Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants