Fix additional re-render when calling fetchMore from useSuspenseQuery when using startTransition.
#11638
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 7d07c58 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
|
/release:pr |
|
A new release has been made for this PR. You can install it with |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
jerelmiller
commented
Mar 4, 2024
| // TODO: Determine why we have this extra render here. This should mimic | ||
| // the update in the next render where we see <App /> included in the | ||
| // rerendered components. | ||
| // Possibly related: https://github.com/apollographql/apollo-client/issues/11315 |
There was a problem hiding this comment.
Narrator: It was related
There was a problem hiding this comment.
Made me laugh. Also, great we're rid of them now!
jerelmiller
commented
Mar 4, 2024
jerelmiller
commented
Mar 4, 2024
| query letters($limit: Int, $offset: Int) { | ||
| letters(limit: $limit) { | ||
| query LettersQuery($limit: Int, $offset: Int) { | ||
| letters(limit: $limit, offset: $offset) { |
There was a problem hiding this comment.
This offset as a variable is important for cache reasons and it was forgotten 😱. Thankfully no tests were harmed in this change, but it was necessary to reproduce the bug in this PR.
|
Hmmm, I can kind of confirm the bug is fixed, but in “rare conditions”, I can trigger a race condition. It is not exactly clear when and how, but it seems that when I spam fetchMore, in some cases, the commit is done later and Apollo struggles to merge everything, leading to a loop where it is trying to fetch indefinitely. Here is the console.log of our app where it does that: Console log from the browserAm trying to create or find a reproducer. In our case, this is an infinite scroll page, so calls are expected to be spammed when scrolling fast |
|
@Arno500 just to clarify, are you sure these are related? This seems like it might be a different issue than reported in #11315, but perhaps I'm misunderstanding something. Based on the original reproduction which demonstrates that an additional rerender is happening, this PR seems to fix that issue. If you think this is different enough, would you mind opening a new issue that we can track separately? A reproduction as you mentioned would also go a long way to help us figure that out. |
|
Hmmmm, to be honest, I don't really know, but it seems the issue seems to be from the same system. Here is a reproducer, so you can take a look at the behavior and tell me if it needs another issue: https://stackblitz.com/edit/stackblitz-starters-t7c2zv?file=src%2Fcomponents%2FPaginatedQuery.tsx,src%2Fapp%2Flayout.tsx It did show before your patch (apparently) but it seems vaguely linked in some ways and appeared on the same project I was working with @pvandamme showing the aforementioned issue. |
|
Apologies, I might be missing something, but I'm not seeing what you're describing above, so perhaps this is a different bug 🤷♂️? What I do see in that reproduction is that the first item is fetched, then the 2nd item via I think this might be a separate issue. It would definitely help to have any instructions/expected behavior in there that way any fix we have for this can be validated against that reproduction. |
|
Alright, in that case I will create a new issue! |
|
At least there's some good news there! Thanks a bunch ahead of time 🙂 |
phryneas
approved these changes
Mar 5, 2024
Comment on lines
10081
to
10091
| { | ||
| const { snapshot, renderedComponents } = await Profiler.takeRender(); | ||
|
|
||
| expect(renderedComponents).toStrictEqual([App]); | ||
| expect(snapshot.result?.data).toEqual({ | ||
| letters: [ | ||
| { __typename: "Letter", letter: "A", position: 1 }, | ||
| { __typename: "Letter", letter: "B", position: 2 }, | ||
| ], | ||
| }); | ||
| } |
There was a problem hiding this comment.
Reading the test, it's not immediate apparent why this rerender happens. I'd assume to disable the button - but it might be good in that case to do an expect(button).toBeDisabled to make that obvious.
jerelmiller
force-pushed
the
issue-11315-fetchmore-suspense
branch
from
9705eb2 to
7d07c58
Compare
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #11315
Due to the mechanics of
fetchMore, the cache broadcast happens after the concast promise is resolved. This interferes with our check on promise resolution to see if we need to emit the result to avoid some situations where the promise might otherwise stay pending forever (#11086). Unfortunately this meant that we emitted a result, then the cache result was broadcasted resulting in an additional re-render.This fix adds a
setTimeoutaround the check to try to allowQueryInfo.notifyto run first, which is itself run inside asetTimeout.