DevTools doesn't remember the sort function

[targumon]

Describe the bug

The DevTools sort function is saved in localStorage (key reactQueryDevtoolsSortFn)
But never really loaded from it (it's always set back to its fallback value: "Status > Last Updated").

Your minimal, reproducible example

N/A (I'll provide it if you insist, but I already tracked the cause, so I deem it unnecessary)

Steps to reproduce

1. Open DevTools
2. Change the sort function to something other than the default (e.g. "Query Hash")
3. Refresh the app (or close its tab, and open it in a new one)

Expected behavior

The selected sort function should persist.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

N/A

react-query version

3.35.0

TypeScript version

4.6.3

Additional context

At Line 398 we load the sort function label from local storage (with a fallback value in case it isn't set):
const [sort, setSort] = useLocalStorage('reactQueryDevtoolsSortFn', Object.keys(sortFns)[0])

In L410, we get the actual function based on that label:
const sortFn = React.useMemo(() => sortFns[sort as string], [sort])

Finally, in L412, if the function is undefined, we save the fallback value to local storage: React[isServer ? 'useEffect' : 'useLayoutEffect'](() => {if (!sortFn) { setSort(Object.keys(sortFns)[0] as string) }}, [setSort, sortFn])

😇 Looks innocent, doesn't it?

😈 The devil is in the details.

One of them is that useLocalStorage "breaks its contract".

1. For starters, it works asynchronously*: reading from storage is inside a useEffect so during the FIRST PASS of the above flow, sort is never what was previously stored.

2. Additionally, the internal state of this custom hook is not initialized with the fallback value (L19).

The result of these two points is that sort starts as undefined.

Now we're at the mercy of which effect will run first. Guess what? it depends.
When isServer is true, we just have 2 useEffect calls and although I didn't test it, they're supposed to be run in the same order they appear in the code.
But when isServer is false, it is useLayoutEffect we're talking about, and it will run BEFORE useEffect.
Inside, the effect sees that sortFn is undefined and therefore it sets it with the fallback value... 🤦

Bottom line: the value is overwritten before it has a chance of being read.

---

*Why? I'm probably lacking some context, but useLocalStorage looks over-engineered to me. I created a similar service in my project and it doesn't use hooks. It operates synchronously with the storage just fine.
If this was done to reduce localStorage reads (does it hit performance?) please note that useState can receive a function, so I believe you can just move the effect code in there (From React docs: lazy initial state).

I'm probably also lacking context for the need to useMemo which again looks over-engineered to me. sortFns is a very small plain "static" object. What exactly are we avoiding/gaining here?

One last comment I have about this code: I didn't "git blame" and I might be totally wrong, but the fact that there are 2 contradicting assumptions in this code (1. useLocalStorage always returns a value, either the one previously stored or the provided fallback; 2. sort & sortFn might be undefined;) gives me the impression there was already a bug in this code and it was worked-around instead of being fixed.

I hope this explains why I opened an issue instead of a pull request - this can be solved in many ways and I don't know which one you'd prefer. Please guide me and I'll be more than happy to contribute.

Thanks for reading so far and for this amazing library! 🙏

[TkDodo]

seems like a bug, feel free to contribute a fix.

It operates synchronously with the storage just fine.

Just keep in mind that we can't read synchronously from localStorage because it will be undefined on the server and thus likely yield hydration warnings. However, I think maybe we can, because we've recently disabled server rendering for the devtools anyways - they now render on the client only 🤔

[zorzysty]

I took a look at it and I can confirm that this effect is causing the problem. Good news is that it's also completely unneeded as it's doubling what's already done via useLocalStorage. And if the effect runs after (which it usually does), it disregards the value that's read by useLocalStorage hook - which is exactly what causes the bug.

This is easily fixable by simply removing the effect. PR open: #3703