rtk-incubator / rtk-query

Data fetching and caching addon for Redux Toolkit
https://redux-toolkit.js.org/rtk-query/overview
MIT License
626 stars 31 forks source link

Reset promiseRef on useQuery hook #173

Closed msutkowski closed 3 years ago

msutkowski commented 3 years ago

Shrugsy reported this in Discord, and this solves it, but I haven't considered any other implications yet. Will review again later.

Is there anything I can do about hot reloading causing queries to get unsubscribed and become 'uninitialized'? On a fresh CRA install, after adding a small rtk query setup, the behaviour I observe is like so:

  1. Launch app (npm start)
  2. Open console
  3. Open redux devtools
  4. [in devtools + console] Observe that data fetches normally (query fulfils in devtools, data & statuses populate in console)
  5. [in ide] Edit and save this file (e.g. edit the logging text) to trigger a hot reload
  6. [in devtools] Observe that the 'pokemonApi/subscriptions/unsubscribeResult' action is dispatched after the hot reload
  7. [in console] Wait 60 seconds (or length of 'keepUnusedDataFor' option)
  8. [in devtools] Observe that the 'pokemonApi/queries/removeQueryResult' action is dispatched
  9. [in console] Observe that 'isUninitialized' is now back to true A repo that reproduces this is here: https://github.com/Shrugsy/simple-rtk-app Editing this file will allow you to reproduce it with the above steps: https://github.com/Shrugsy/simple-rtk-app/blob/master/src/features/pokemon/PokemonItem.jsx
github-actions[bot] commented 3 years ago

size-limit report 📦

Path Size
ESM full 9.76 KB (0%)
ESM full (React) 10.84 KB (+0.06% 🔺)
createApi + setupListeners 8.88 KB (0%)
createApi (React) + setupListeners 9.85 KB (+0.06% 🔺)
fetchBaseQuery 661 B (0%)
retry 271 B (0%)
ApiProvider 400 B (0%)
CJS minfied 15.13 KB (0%)
CJS React minfied 16.39 KB (+0.05% 🔺)
codesandbox-ci[bot] commented 3 years ago

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 08ef189c0d1e92aa1116dbf2b467066b971a42ac:

Sandbox Source
React Configuration
React Typescript Configuration
rtk-query-demo Configuration
svelte-app-rtk-simplequery-demo Configuration
phryneas commented 3 years ago

I'm not 100% sure this is the "most correct solution".

From what I gather, this is caused by the cleanup callback being run as if the component was unmounted. But after that, the other effect does not act like the component was re-mounted. This is because we never clean the promiseRef in the cleanup callback and then compare it with a shallowly memoized value and see "no change". https://github.com/rtk-incubator/rtk-query/blob/next/src/react-hooks/buildHooks.ts#L192

So I think a better fix would be to reset the refs in the cleanup callback in https://github.com/rtk-incubator/rtk-query/blob/next/src/react-hooks/buildHooks.ts#L216-L218

msutkowski commented 3 years ago

I'm not 100% sure this is the "most correct solution".

From what I gather, this is caused by the cleanup callback being run as if the component was unmounted. But after that, the other effect does not act like the component was re-mounted. This is because we never clean the promiseRef in the cleanup callback and then compare it with a shallowly memoized value and see "no change". https://github.com/rtk-incubator/rtk-query/blob/next/src/react-hooks/buildHooks.ts#L192

So I think a better fix would be to reset the refs in the cleanup callback in https://github.com/rtk-incubator/rtk-query/blob/next/src/react-hooks/buildHooks.ts#L216-L218

I agree. I think the one counterpoint though is that if someone dispatched updateSubscriptionOptions after scheduling a removal in a non-hook implementation, the original solution in middleware would be a 'safety net'. Then again, middleware probably shouldn't be concerned with that? :shrug:

phryneas commented 3 years ago

@msutkowski it is a valid concern, but I think we should catch that case over in https://github.com/rtk-incubator/rtk-query/blob/ef4208904d46fbe4fbbf505e0e8a78d5670e149b/src/core/buildInitiate.ts#L125-L127 and either do nothing or even throw an exception?