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

Allow useLazyQuery trigger fn to change query #10499

Merged
merged 4 commits into from
Feb 1, 2023

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Feb 1, 2023

fixes #10496

Locally, many of the tests were erroring out for me - but independently of the useLazyQuery tests and even without my changes. So let's see what CI has to say about this.

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

@changeset-bot
Copy link

changeset-bot bot commented Feb 1, 2023

🦋 Changeset detected

Latest commit: ac72e11

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

@@ -13,7 +13,7 @@ type OptionsUnion<TData, TVariables extends OperationVariables, TContext> =
| MutationOptions<TData, TVariables, TContext>;

export function mergeOptions<
TOptions extends OptionsUnion<any, any, any>
TOptions extends Partial<OptionsUnion<any, any, any>>
Copy link
Member Author

@phryneas phryneas Feb 1, 2023

Choose a reason for hiding this comment

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

This was necessary since LazyQueryHookOptions does not 100% match any member of OptionsUnion and this way it felt cleaner than adding LazyQueryHookOptions to the union (which is in the shared utilities folder and has no direct relation to the React hooks). This actually fixes a few things where before, mergeOptions would return a OptionsUnion instead of the correct type.

@alessbell
Copy link
Member

Locally, many of the tests were erroring out for me - but independently of the useLazyQuery tests and even without my changes. So let's see what CI has to say about this.

We've pushed a few things to improve the reliability of our test suite recently that have helped, but unfortunately a flaky test still failed here, along with the bundlesize check which seems to be narrowly above the limit (feel free to increase it in bundlesize.ts).

I suspect that on the next CI run this will be 🍏

@phryneas
Copy link
Member Author

phryneas commented Feb 1, 2023

Yup, now it's green 🎉

Copy link
Member

@alessbell alessbell left a comment

Choose a reason for hiding this comment

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

🚀

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.

Nice! I had one minor nit for the changelog verbage, but otherwise this looks great. Welcome to the team 🎉 🎉 🎉

.changeset/seven-cameras-kiss.md Outdated Show resolved Hide resolved
phryneas and others added 2 commits February 1, 2023 19:27
@phryneas phryneas merged commit 9e54f5d into apollographql:main Feb 1, 2023
@github-actions github-actions bot mentioned this pull request Feb 1, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 4, 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.

useLazyQuery ignores changes to the query document
3 participants