Skip to content
This repository has been archived by the owner on Nov 23, 2024. It is now read-only.

Reset promiseRef on useQuery hook #173

Merged
merged 3 commits into from
Mar 16, 2021
Merged

Reset promiseRef on useQuery hook #173

merged 3 commits into from
Mar 16, 2021

Conversation

msutkowski
Copy link
Member

@msutkowski msutkowski commented Mar 16, 2021

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

@msutkowski msutkowski added the bug Something isn't working label Mar 16, 2021
@msutkowski msutkowski requested a review from phryneas March 16, 2021 01:15
@github-actions
Copy link

github-actions bot commented Mar 16, 2021

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
Copy link

codesandbox-ci bot commented Mar 16, 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 08ef189:

Sandbox Source
React Configuration
React Typescript Configuration
rtk-query-demo Configuration
svelte-app-rtk-simplequery-demo Configuration

@phryneas
Copy link
Collaborator

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 msutkowski changed the title Clear removal timeout when updating subscription options Reset promiseRef on useQuery hook Mar 16, 2021
@msutkowski msutkowski force-pushed the fast-refresh-support branch from ad20449 to d20b646 Compare March 16, 2021 15:11
@msutkowski
Copy link
Member Author

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? 🤷

@phryneas
Copy link
Collaborator

@msutkowski it is a valid concern, but I think we should catch that case over in

updateSubscriptionOptions(options: SubscriptionOptions) {
dispatch(updateSubscriptionOptions({ endpointName, requestId, queryCacheKey, options }));
},
and either do nothing or even throw an exception?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants