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

Fix some cases where the auto dispose timeout could cause components to suspend indefinitely #11274

Merged
merged 7 commits into from
Oct 9, 2023

Conversation

jerelmiller
Copy link
Member

Fixes #11270

The autoDisposeTimeoutMs configurable value for Suspense hooks was created to ensure components that suspend but never remount would get properly cleaned up after the timeout duration. This works well to cleanup after components that never mount again after initially suspending (such as a user interaction that hides the element while suspended). This timer started when the query ref was initialized and disposed of regardless of whether the initial promise had settled or not. This meant that when the request finished after the configured timeout, the component would suspend indefinitely since nothing was around to resolve/reject the promise leaving it in a pending state forever.

This PR adjusts the start of the dispose timer to wait until the initial promise is settled. This ensures React will try rendering again regardless of how long the request takes, and only after autoDisposeTimeoutMs will the query ref try and clean itself up if a component doesn't come along and retain it.

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

@jerelmiller jerelmiller added 🐞 bug 🥚 backwards-compatible for PRs that do not introduce any breaking changes ⏲️ react-suspense labels Oct 5, 2023
@changeset-bot
Copy link

changeset-bot bot commented Oct 5, 2023

🦋 Changeset detected

Latest commit: 2687f8f

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

@jerelmiller jerelmiller requested a review from phryneas October 5, 2023 23:43
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 37.07 KB (+0.06% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 43.4 KB (+0.05% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 41.96 KB (+0.06% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 32.47 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production) 31.22 KB (0%)
import { ApolloProvider } from "dist/react/index.js" 1.22 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production) 1.21 KB (0%)
import { useQuery } from "dist/react/index.js" 4.28 KB (0%)
import { useQuery } from "dist/react/index.js" (production) 4.1 KB (0%)
import { useLazyQuery } from "dist/react/index.js" 4.59 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production) 4.41 KB (0%)
import { useMutation } from "dist/react/index.js" 2.54 KB (0%)
import { useMutation } from "dist/react/index.js" (production) 2.52 KB (0%)
import { useSubscription } from "dist/react/index.js" 2.24 KB (0%)
import { useSubscription } from "dist/react/index.js" (production) 2.2 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" 4.62 KB (+0.47% 🔺)
import { useSuspenseQuery } from "dist/react/index.js" (production) 4.05 KB (+0.54% 🔺)
import { useBackgroundQuery } from "dist/react/index.js" 4.14 KB (+0.53% 🔺)
import { useBackgroundQuery } from "dist/react/index.js" (production) 3.56 KB (+0.64% 🔺)
import { useReadQuery } from "dist/react/index.js" 3 KB (+0.73% 🔺)
import { useReadQuery } from "dist/react/index.js" (production) 2.94 KB (+0.61% 🔺)
import { useFragment } from "dist/react/index.js" 2.09 KB (0%)
import { useFragment } from "dist/react/index.js" (production) 2.04 KB (0%)

@jerelmiller
Copy link
Member Author

/release:pr

@jerelmiller jerelmiller changed the title Start the auto dispose timeout for suspense-enabled components from suspending indefinitely when the request duration is longer than timeout duration Fix some cases where the auto dispose timeout could cause components to suspend indefinitely Oct 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

A new release has been made for this PR. You can install it with npm i @apollo/[email protected].

Copy link
Member

@phryneas phryneas left a comment

Choose a reason for hiding this comment

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

This looks good to me! :)

@jerelmiller jerelmiller merged commit b29f000 into main Oct 9, 2023
@jerelmiller jerelmiller deleted the issue-11270-uSQ-timeout branch October 9, 2023 21:55
This was referenced Oct 9, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🥚 backwards-compatible for PRs that do not introduce any breaking changes 🐞 bug ⏲️ react-suspense
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useSuspenseQuery leaves component suspended if fetch is aborted
3 participants