-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Clean up subscriptions using requestIdleCallback instead of setTimeout #11228
Clean up subscriptions using requestIdleCallback instead of setTimeout #11228
Conversation
@raymondwang: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
|
Name | Link |
---|---|
🔨 Latest commit | 6cd5907 |
🦋 Changeset detectedLatest commit: 6cd5907 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 |
531a959
to
6cd5907
Compare
Hey @raymondwang 👋 Since it looks like you're wanting to experiment with whether this change improves performance, I'll go ahead and create a snapshot release you can use to install this change. Let us know if it helps! |
/release:pr |
@@ -17,6 +17,7 @@ import { | |||
fixObservableSubclass, | |||
getQueryDefinition, | |||
} from "../utilities/index.js"; | |||
import { requestIdleCallback } from "../utilities/common/requestIdleCallback.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I polyfilled this for compatibility with other environments (including Jest…), but exposing it via the public API for utilities seemed like overkill. Let me know if this feels like the wrong place and I'm happy to move things around.
A new release has been made for this PR. You can install it with |
Thanks @jerelmiller! Just tested this solution against my environment with a couple thousand subscriptions and… this change seems to make things dramatically worse. Individual unsubscribe tasks look like they're going from 0.2 - 0.5 ms to around 1.5 - 2 ms — unacceptably slow. We're converting our query hooks to |
Dang thats a bummer! Appreciate you taking the time to try some things out! Let me know if/when you have some changes you'd like to test against a snapshot release and I or one of the team would be happy to kick off the workflow again. |
Hey @raymondwang 👋 I'm doing a bit of a cleanup on open PRs and noticed there hasn't been much activity lately. I'm going to go ahead and close this PR. If you'd like to continue experimenting, feel free to reopen this PR and submit changes. At the very least, thanks for the exploration thus far! |
Checklist:
Referenced in #11214.