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

allow for "shared component results" using the useMutation hook #1477

Merged
merged 9 commits into from
Oct 16, 2021

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Sep 3, 2021

This needs docs to be written, but should be ready otherwise.

It would be used like

const [trigger, result] = useMyMutation({ fixedCacheKey: "myCacheKey" })

That cache key would be shared over components and never cleaned up, so the result would stay around until either result.reset() or trigger are called (the first removing the result, the second overwriting it with new results).

The only caveat: When using fixedCacheKey, originalArgs will always be undefined. We cannot reliably guarantee that to be there in any situation, since we keep them out of the store (for mutations, they are often non-serialiazable) and we cannot keep them "to the side" as then they would not be able to be restored in a persistence or SSR situation, so we explicitly set them to undefined as soon as fixedCacheKey is being used.

The persistence problem even exists without fixedCacheKey, so we should maybe even think about generally deprecating them and encourage people to hold originalArgs to the side manually if they need them for something.

Installation instructions are in the CodeSandbox build page.

@netlify
Copy link

netlify bot commented Sep 3, 2021

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

🔨 Explore the source changes: 4b389bf

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

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 3, 2021

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 6db0429:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
rsk-github-issues-example Configuration
@examples-query-react/basic Configuration
@examples-query-react/advanced Configuration

@github-actions
Copy link

github-actions bot commented Sep 3, 2021

size-limit report 📦

Path Size
1. entry point: @reduxjs/toolkit (cjs.production.min.js) 12.28 KB (0%)
1. entry point: @reduxjs/toolkit (esm.js) 10.27 KB (0%)
1. entry point: @reduxjs/toolkit/query (cjs.production.min.js) 20.51 KB (+0.44% 🔺)
1. entry point: @reduxjs/toolkit/query (esm.js) 17.51 KB (+0.43% 🔺)
1. entry point: @reduxjs/toolkit/query/react (cjs.production.min.js) 22.57 KB (+0.58% 🔺)
1. entry point: @reduxjs/toolkit/query/react (esm.js) 20.14 KB (+0.7% 🔺)
2. entry point: @reduxjs/toolkit (without dependencies) (cjs.production.min.js) 5.6 KB (0%)
2. entry point: @reduxjs/toolkit (without dependencies) (esm.js) 5.58 KB (0%)
2. entry point: @reduxjs/toolkit/query (without dependencies) (cjs.production.min.js) 9.31 KB (+1% 🔺)
2. entry point: @reduxjs/toolkit/query (without dependencies) (esm.js) 9.67 KB (+0.8% 🔺)
2. entry point: @reduxjs/toolkit/query/react (without dependencies) (cjs.production.min.js) 2.59 KB (+2.79% 🔺)
2. entry point: @reduxjs/toolkit/query/react (without dependencies) (esm.js) 2.43 KB (+3.24% 🔺)
3. createSlice (esm.js) 5.16 KB (0%)
3. createEntityAdapter (esm.js) 5.83 KB (0%)
3. configureStore (esm.js) 5.83 KB (0%)
3. createApi (esm.js) 15.8 KB (+0.41% 🔺)
3. createApi (react) (esm.js) 18.38 KB (+0.7% 🔺)
3. fetchBaseQuery (esm.js) 10.98 KB (0%)
3. setupListeners (esm.js) 9.86 KB (+0.01% 🔺)
3. ApiProvider (esm.js) 17.22 KB (+0.4% 🔺)

@phryneas phryneas marked this pull request as ready for review October 15, 2021 19:08
@phryneas phryneas merged commit 762ee8a into v1.7.0-integration Oct 16, 2021
Comment on lines +827 to +838
const reset = useCallback(() => {
if (promise) {
setPromise(undefined)
} else if (fixedCacheKey) {
dispatch(
api.internalActions.removeMutationResult({
requestId,
fixedCacheKey,
})
)
}
}, [dispatch, fixedCacheKey, promise, requestId])
Copy link
Collaborator

@Shrugsy Shrugsy Oct 16, 2021

Choose a reason for hiding this comment

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

💁

I think this logic makes it so that when using a fixed cache key, the component that calls reset needs to call it twice before it actually removes the result for that key

e.g.

  • Components A & B with a shared mutation key
  • Component A triggers the mutation
  • Component A stores the promise in state
  • Components A & B both receive the mutation result
  • Component A calls reset
  • The promise state in component A is set to undefined, but the mutation result for the cache key is left alone
  • Components A & B both still receive the mutation result
  • Component A calls reset again
  • The mutation result for the cache key is now removed

My immediate thought is to change the else if (fixedCacheKey) to just if (fixedCacheKey)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. My thought was: Setting promise to undefined should trigger the useEffect cleanup further up, which also triggers a reset.

But unfortunately, that skips in that case:

      useEffect(
        () => () => {
          if (!promise?.arg.fixedCacheKey) {
            promise?.reset()
          }
        },
        [promise]
      )

Could you open a PR against the integration branch and maybe also add a test for this to prevent regressions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yup, no problem

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants