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

Hotfixes #9328

Merged
merged 6 commits into from
Jan 21, 2022
Merged

Hotfixes #9328

merged 6 commits into from
Jan 21, 2022

Conversation

brainkim
Copy link
Contributor

@brainkim brainkim commented Jan 19, 2022

Review by commit.
Fixes #9101, #9129, #9142.

As a way to simplify useLazyQuery(), we are now using the skip: true/refetch pattern described in #9101 to define useLazyQuery(), which should give us useMutation() like error behavior.

useEffect() just should not be trusted to do anything at all.

@brainkim brainkim requested a review from benjamn January 19, 2022 21:23
@benjamn benjamn added this to the v3.5.x patch releases milestone Jan 19, 2022
Comment on lines +424 to +426
expect(errorMock).toHaveBeenCalledTimes(1);
expect(errorMock.mock.calls[0][0]).toMatch("Missing field");
errorMock.mockRestore();
Copy link
Contributor

@sztadii sztadii Jan 19, 2022

Choose a reason for hiding this comment

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

I think we should not do the below assertions

expect(errorMock).toHaveBeenCalledTimes(1);
expect(errorMock.mock.calls[0][0]).toMatch("Missing field");

Those elements are part of validation that is not tested here.
Otherwise if someday we will going to change the validation message from
Missing field to The field "name" is missing we will need to update much more tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mostly a sanity check to make sure another error hasn’t snuck into the test. The tests would definitely fail if we changed the error message but I’m okay with it because that would be a quick fix.

@benjamn benjamn force-pushed the brian-3.5-triage-2 branch from 4e72cbb to 8e0e7a2 Compare January 20, 2022 01:46
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

LGTM with a few small questions. Thanks @brainkim!

src/react/hooks/useLazyQuery.ts Show resolved Hide resolved
src/react/hooks/useLazyQuery.ts Outdated Show resolved Hide resolved
src/react/hooks/useLazyQuery.ts Outdated Show resolved Hide resolved
@brainkim brainkim merged commit 147c286 into main Jan 21, 2022
@brainkim brainkim deleted the brian-3.5-triage-2 branch January 21, 2022 10:36
@efstathiosntonas
Copy link
Contributor

thanks for the stale variables fix, this was driving me crazy and I was thinking it was me.

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.

Refetch no longer works in 3.5 when skip is true
4 participants