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

Add upsertQueryData functionality per issue #1720 #2007

Closed

Conversation

lindapaiste
Copy link
Contributor

Is this the desired functionality?

  • Should a user be allowed to manually insert an error, or just data?
  • Should the third argument of the action creator be a function which is called with the previous data, even though that data might be undefined?
  • Are the returned immer patches correct? I will need help writing the unit tests on these.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 7, 2022

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 ff325ae:

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

@netlify
Copy link

netlify bot commented Feb 7, 2022

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

🔨 Explore the source changes: ff325ae

🔍 Inspect the deploy log: https://app.netlify.com/sites/redux-starter-kit-docs/deploys/6200733e096b69000725a5e7

😎 Browse the preview: https://deploy-preview-2007--redux-starter-kit-docs.netlify.app

@markerikson
Copy link
Collaborator

Oh hey, very cool! Thanks for putting this together!

I'll defer to @phryneas , but at least at first glance this looks like basically what I imagine this feature is supposed to do (and I see you based on the implementation off the suggested Immer patches approach in #1720 ).

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.

The problem I see here conceptually is that this adds data, but will never trigger onCacheEntryAdded. And since that cache entry now exists, onCacheEntryAdded will also not be called in the future for this cache entry.

So, at this moment I think this might not be the right way of going about this. We would have to at least trigger the thunk so the lifecycle actions would be dispatched for the middlewares to pick it up correctly.

And if we only trigger the thunk, overwriting the baseQuery/queryFn call, it might still not be complete, since initiate contains logic to hold the "currently running request" for concurrecy reasons - and this will take a few ticks to completely go through all steps, so that should really be held.

=> it might make sense to add an internal option forceQueryFnto initiate that passes it on to queryThunk. That way it would behave exactly the same in terms of lifecycle handling.

So the implementation of upsertQueryData would become something like

const forceQueryFn = Symbol();
const upsertQueryData = (endpointName, args, data) => (dispatch, getState) => {
  // get initiate for the right endpoint here
  dispatch(initiate(arg, { subscribe: false, forceRefetch: true, [forceQueryFn]: () => ({ data }) }))
}

The downside is that this would put all queries in a loading state, so there it could again switch between updateQueryData and inititate depending on context.

draft[queryCacheKey] = {
status: QueryStatus.fulfilled,
endpointName,
requestId: '',
Copy link
Member

Choose a reason for hiding this comment

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

I don't feel particiular good about having this as an empty string.If we keep going about it like this, let's create a nanoid id in upsertQueryData and pass it in here as part of the action payload.

@phryneas
Copy link
Member

phryneas commented Jul 3, 2022

This is pretty much superseded by #2266, so I'm gonna close this one - but thanks a lot for the PR and the initiative!

@phryneas phryneas closed this Jul 3, 2022
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.

3 participants