Added useQuery frame tests to show that there is an unneeded frame. #9508
Conversation
|
@FritsvanCampen Were you intending this test to fail? Or is it just a (currently passing) regression test? |
The intention was that the test passes and showing that there is an 'unneeded' frame. Regardless of whether we want to spend efforts on trying to eliminate it, I think it's good to document that this how the hook currently behaves. In it's current state the tests also serve as a regression test for any unintentional 'new' frames that might be emitted if the implementation is changed. So this PR could be merged as-is, it's merely documenting the current behavior. I think it would be good to expand these tests for other cases. I tried making a test for The tests can be written passing with PS. |
|
Okay, great! I'm all for adding more passing tests that help codify the current behavior (as long as they're not difficult to update when we deliberately adjust the behavior). |
| await waitFor(() => result.current.loading === false); | ||
| rerender({ children: null }); | ||
| expectFrames(result, [ | ||
| { loading: true, data: void 0 }, | ||
| { loading: false, data: { hello: "world" } }, | ||
| UNNEEDED_FRAME | ||
| ]); |
There was a problem hiding this comment.
@FritsvanCampen Looking more closely at this code, the extra frame results from calling rerender({ children: null }) after waiting for result.current.loading to be false (which accounts for the first two results).
If I understand correctly, since you're calling the React rerender function here, the renderHook is definitely going to run again, so there's not much Apollo Client can do to keep useQuery from being called again when the rerender happens.
What are you proposing useQuery should do when it's called in that case? Returning the same object it returned last time (the UNNEEDED_FRAME) seems like a reasonable behavior, no?
There was a problem hiding this comment.
Well spotted, I think you're right. In this case there is no UNNEEDED_FRAME. I think I made a mistake copy-pasting from the referential stability test. There still an UNNEEDED_FRAME in the other test though.
benjamn
changed the base branch from
useQuery-internal-state-ref
to
release-3.6
This removes the UNNEEDED_FRAME from the second test added in PR #9508, which currently causes the test to fail, though the version without the UNNEEDED_FRAME is how we ultimately want useQuery to behave. I believe the other test is correct to have the repeated frame: #9508 (comment)
Final cleanup to address #9508.
|
Sorry for the accidental close! This should soon be fixed (at last) by #9599. |
…-frame Fix extra `useQuery` result frames (as demonstrated by #9508).
|
This should be fixed now in npm i @apollo/client@betawhen you have the chance! We are targeting the final release for v3.6 for the end of this week. |
|
The |
PR related to #9459 (review)