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

Removing onCompleted and onError from the useQuery types #2645

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexandcote
Copy link
Contributor

Description

Our custom implementation of useQuery and <Query /> doesn't support onCompleted and onError.

This PR update the types to reflect the difference between our API and Apollo API.

@alexandcote alexandcote force-pushed the alexandcote/fix-graphql-type branch from 230ce1c to d00962d Compare May 2, 2023 19:58
@alexandcote alexandcote marked this pull request as ready for review May 2, 2023 19:58
@alexandcote alexandcote requested a review from a team as a code owner May 2, 2023 19:58
@alexandcote alexandcote requested a review from BPScott May 2, 2023 19:58
@alexandcote alexandcote changed the title Removing and from the useQuery type Removing onCompleted and onError from the useQuery types May 2, 2023
@BPScott
Copy link
Member

BPScott commented May 3, 2023

Change looks reasonable though I'm concerned that folks might have been using these props without realising they do nothing, and thus removing them from the type will introduce a bunch of type errors in web. Could you please cut a snapshot and test if using it in web has an effect on running type-check?

@BPScott
Copy link
Member

BPScott commented May 3, 2023

/snapit

@github-actions
Copy link
Contributor

github-actions bot commented May 3, 2023

🫰✨ Thanks @BPScott! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/react-graphql@0.0.0-snapshot-20230503131115
yarn add @shopify/react-graphql-universal-provider@0.0.0-snapshot-20230503131115

@alexandcote
Copy link
Contributor Author

alexandcote commented May 3, 2023

I see some instances

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Looks like the impact in web is pretty light and if we release this fixing web's CI would be pretty quick. I'm ok with claiming this is a patch release and we have to fix those places in web.

Code changes look good, I've added a suggestion for how to format the changeset so something that would feel a bit nicer to read as a changelog entry.

Alex, can you own getting this updated within web, so we we don't have to do the tidying as part of some future update?

.changeset/breezy-dolphins-exercise.md Outdated Show resolved Hide resolved
@alexandcote alexandcote force-pushed the alexandcote/fix-graphql-type branch 2 times, most recently from db126a3 to 22739ad Compare June 9, 2023 15:37
Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

LGTM. Can you own getting this updated in web (I've moved onto checkout things).

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.

None yet

2 participants