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

keep subscription on data while query is running #3709

Merged
merged 8 commits into from
Oct 27, 2023

Conversation

phryneas
Copy link
Member

This would fix #3706 and solve a generally long-existing problem: Prior to this PR, unsubscribed initiate calls without an existing cache entry don't have a return value.

The downside is that now every initiate call creates a cache entry, even with subscribe: false - and trigger lifecycle actions.
It might make sense to target 2.0 with this.

@codesandbox
Copy link

codesandbox bot commented Sep 10, 2023

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 10, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6da44eb:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
rsk-github-issues-example Configuration
@examples-query-react/basic Configuration
@examples-query-react/advanced Configuration
@examples-action-listener/counter Configuration
rtk-esm-cra Configuration

@markerikson markerikson added this to the 2.0 milestone Oct 9, 2023
@markerikson markerikson changed the base branch from master to v2.0-integration October 19, 2023 13:55
@netlify
Copy link

netlify bot commented Oct 19, 2023

Deploy Preview for redux-starter-kit-docs ready!

Name Link
🔨 Latest commit 6da44eb
🔍 Latest deploy log https://app.netlify.com/sites/redux-starter-kit-docs/deploys/653b1c43ff5324000800321d
😎 Deploy Preview https://deploy-preview-3709--redux-starter-kit-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@markerikson
Copy link
Collaborator

I'm admittedly hesitant on this one because I don't like anything that involves dispatching more actions. But both of you seem to think this is a good idea, so we'll go with it. Retargeted for 2.0 and rebased.

@markerikson
Copy link
Collaborator

I tried doing a perf check on this, and I'm pretty concerned.

here's the "Invalidate" reload activity section for each, beta.3 on the left , PR on the right. Note that beta.3 is like 2800 ms (WHICH IS STILL REALLY BAD) and PR is 6000+ seconds (ACK):

image

The issue here is that every dispatching is running in a microtask and taking up another 2-5ms. I haven't counted the number of actions, but the amount of work here is going up significantly somehow.

This is sort of tied into the way we're mutating the subscription data in the RTKQ middleware, and then trying to track that and generate diffs via JSON.parse(JSON.stringify()) before and after so that we can dispatch(subscriptionsUpdated(patches)).

At this point I'm inclined to kick this to "Post 2.0" (and likely to RTK 3.0) on the grounds that this needs more time to be thought through, and I'm trying to be pretty ruthless about cutting scope to get this out the door.

- Changed `subscriptionUpdated` from a microtask to a 500ms throttle
- Exposed the RTKQ internal middleware state to be returned via an
  internal action, so that hooks can read that state directly without
  needing to call `dispatch()` on every render.
- Reworked tests to completely ignore `subscriptionsUpdated` in any
  action sequence checks due to timing changes and irrelevance
- Fixed case where `invalidateTags` was still reading from store state
@markerikson
Copy link
Collaborator

Did a bunch of work on top of this in #3824 around changing how often we sync the subscription state, and also reworked how hooks access the current subscription data. Perf seems equivalent now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: UseLazyXXXQuery Returning undefined On Long Running Queries
3 participants