useForceUpdate hook unreliable with React 18.2 on Safari, due to mounted check via effect

[gkaindl]

When porting our app to React 18, I noticed that the useQuery() hook will sometimes not update with the resolved query, but only on Safari (16.5) in production mode, and only if used from within a state transition (startTransition() or useTransition())

The fetch does successfully finish on the network, though.

I managed to track this down to the useForceUpdate() hook that's part of relay-hooks, and specifically commit a82fd1e, where a check has been added to only actually force an update if the component is mounted, which is tracked through an effect.

However, on Safari, we run into the situation where forceUpdate() should be called (due to the query resolving) before the effect hook has fired, thus dropping the update and never delivering the resolved query from useQuery() (it appears as if the query starts loading, but never finishes).

This seems to be related to other Safari/React 18 issues, such as facebook/react#26713 or facebook/react#24458, and we've been running into similar subtle timing/ordering issues on Safari with React 18 (and especially transitions) ourselves too, which do not appear on Blink-based browsers or Firefox.

I've implemented an extended version of the "do not call forceUpdate unless mounted" fix, which tracks a pending forceUpdate call and immediately executes it in the "mount" effect, so that it becomes reliable to call forceUpdate before the effect fires: gkaindl@3311fce – EDIT: And with a fresh mind in the morning actually shortened to 0b229c7.

EDIT 2: And after a week of running this in a branch of our app, back to a variation of the first version: gkaindl@4bb7326 – When the time-slicing approach of React18 transitions is used, and since the forceUpdate() function is passed to "outside of the React world", it's actually possible for the function to be called not only after the component is mounted, but before the effect to set isMounted = true triggers, but also in between of the component being rendered, but before the new fibers are flushed (especially if a lot of stuff (re-)renders in the transition, and the query is served from the relay store without hitting the network), which causes a "Can't perform a React state update on a component that hasn't mounted yet." warning in dev mode. Thus, actually tracking the "pending" forceUpdate in a ref and triggering it during the effect is safer in this case.

This fixes the problem on Safari/React18 for me (all tests pass, too), and I'll gladly submit this as a pull request if you agree.

Unfortunately, I can't provide a repro case publicly, but could set one up that I can privately share via email, if you're interested (my github user name at mac dot com), where the issue triggers fairly reliably (e.g. once for every 2-3 page reloads, at least on my machine).

Also, many thanks for providing and maintaining relay-hooks! It's been invaluable for us in transitioning our app to hooks-based data loading while avoiding Suspense, which wouldn't have been a good fit for us before React 18's state transitions.

[morrys]

I'am on holiday, i will take a look next week:)

[gkaindl]

Of course! Enjoy your holidays! 🙂🌴

[eMerzh]

Hello Here...
it seems i have a similar issue.. but not on safari
the symptoms are: usequery on a page, 👌 , then navigate away, then navigate back to there, same usequery, return "data=null" & isLoading = true...
never updating to data=something ...
if i manage to do a "forceUpdate" manually, the data get filled in 🤔

note: i've downgrade to 7.2.1 , where the problem doesn't seems to be present and it works

[morrys]

Hi @eMerzh,
are you using the suspense version too?
could you try to recreate the error case in an example project or modify one of those present in this repo?

[eMerzh]

No... No suspense here...
Unfortunately the project is not open... So hard to take the exact code
...

[morrys]

I understand the difficulty, I would like to try to fix it but I can't recreate the problem :/

What dependencies are you using?

[eMerzh]

https://gist.github.com/eMerzh/e0313008d6e672d3efc0c20a2d5ec83f here is a quick list...
i'll try to see if i can extract some code...

[eMerzh]

hum... trying to replicate using the examples from repo (after update).. But no luck for the moment...
even on my app, it's not "all the time" as gkaindl noticed... but relatively repeatable
(also not ALL pages in the app have that.. but some... )

i know it's vague but debugging this in local, it seems that the QueryFetcher correctly set the datas for getData()
but isn't force updated... if i manage to call forceupdate , everything triggers and correct itself

if i log something arround here
https://github.com/relay-tools/relay-hooks/blob/master/src/useQuery.ts#L35C28-L35C28
and i put a onComplete & onResult on the useQuery app code....

(after the inital load, go away, ...)
it displays :

{error: null, data: null, isLoading: true }
in useQuery.ts

then onResponse triggers

{
data: { me: {…}, instance: {…} }
​​extensions: Object { cacheTimestamp: 1701895518319 }
}

then onComplete (no error)

null

and that's it... no useQuery.ts again ...

hope it helps a bit.. i'll continue check if i can go further

[eMerzh]

@morrys oh.. in fact it seems that the complete occurs before the useEffect in https://github.com/relay-tools/relay-hooks/blob/master/src/useForceUpdate.ts#L7 could be enabled triggered,

thus the forceUpdate is called doesn't update...
if useForceUpdate's isMounted check is there to verify that a hooks is unmounted (vs not fully mounted)
you probably want to put the default value of the ref to true, no ?
(doing that it make it work... )

[gkaindl]

@eMerzh Yes, this is exactly the behavior I was seeing too. You could try to temporarily npm-install from github:gkaindl/relay-hooks#install-from-git to see if this fixes it for you too, but yeah, the effects being called in the wrong order, so that the actual update is skipped due to the mounted check, is what I've been seeing too.

Regarding "isMounted initialized with true": It appears that the forceUpdate function is sometimes passed "upwards" in the node tree, so it can then happen that it is being called before the mounting effect triggers, and that causes a warning that you can't set state on a component which isn't mounted yet. Thus, I changed it to use an additional "isPending" flag, which is used to trigger the update when the mounting effect runs.

Regarding "all the time": In our app, it was 100% reproducible on Safari if the component is mounted during a React transition (but not 100% otherwise), but most interestingly, it always worked on Chrome and Firefox.

[eMerzh]

@gkaindl my fixe would be to to flip https://github.com/relay-tools/relay-hooks/blob/master/src/useForceUpdate.ts#L5 the ref to true by default 🤔
did you test that?

[morrys]

@gkaindl I am available to integrate your version, obviously without being able to see the error with my own eyes I am afraid of fixing your problem but introducing others or having solved a problem that is actually react's

I say this mostly because you two are describing the same problem but in two very different contexts

Do you have the same effect with react 17?

[gkaindl]

@eMerzh Yes, but I'm then getting occasional Can't perform a React state update on a component that hasn't mounted yet. React warnings (at least in our app) -> I've tracked this down to the forceUpdate() function sometimes being called from a relay resolver, e.g. "from outside the React world", so it can happen that it is called before the component is really mounted. Therefore, I went with a more involved version which tracks the early calls and then forces the update when the isMounted effect actually triggers.

[gkaindl]

@morrys No, we noticed the issue only when upgrading from 17 to 18, and we didn't ever see it on 17. Also, it seems to be triggered (mostly or only, not sure?) by concurrent rendering, e.g. if the relay resolver resolves while the component that uses the hook is rendering in concurrent mode.

It's possible it's also connected to us using a very fast (edge-cached) API, where queries are often served from the cache in a few milliseconds. I think what's happening for us is that

1. We start a React transition with startTransition().
2. The component using useQuery() renders (in the "background"), but returns null data and isLoading === true.
3. The relay resolver finishes and calls forceUpdate(), while the low-priority transition is still ongoing, so the component isn't mounted yet (its effects haven't triggered yet), so that update gets lost. This can happen because the forceUpdate() function is passed to "outside the React world" here, and we can't rely on the mounting effect triggering before the forceUpdate() call therefore.
4. The transition finishes, and the component's effects trigger, but the update is "lost".

In my branch, I basically store that there was a forceUpdate() in 3 and then trigger it in 4 to work around this.

I understand about the repro. Unfortunately, our app is also closed :-/ I would conjecture that it may be possible to repro by intentionally creating a "slow" transition that takes longer to render in concurrent mode than the API needs to respond, so that the relay resolver triggers during the concurrent render, like a component that runs the useQuery() hook and then renders lots of children, each of them with a loop in them to make the render even slower.

Most interestingly, I also got the error reproducibly when using the browser "back" button, which triggers additional "browser magic" to make the previous state render faster, but in any case, the core issue is that forceUpdate() can be called by the QueryFetcher before the effect setting the isMounted flag has run (that I could verify with breakpoints).

[eMerzh]

i might miss something, but how is it possible to have the issue with the component not mounted yet 🤔
(it's often when unmounted .... )

like if i do

export default function Form() {
  const [u, forceUpdate] = useReducer((x) => x + 1, 0);
  if (u === 0) {
    forceUpdate()
  }
  return u
  }

i don't get the error 🤔 how can it be "before that" 🤔
or it's another issue

[gkaindl]

@eMerzh In concurrent mode, it's possible, because React will only update the DOM when the entire node tree (for the current render pass) has finished rendering, but this can be interrupted by more important updates (they use a time-slicing algorithm to achieve this, which is basically just deferring work to after the next browser frame after a certain time limit).

So what can happen is that the function is returned from the reducer and passed to the relay resolver, but the component is not mounted, because the render pass is not yet finished. Then the time slicing algorithm interrupts the pass, giving the relay resolver time to already call that forceUpdate function, and in this case, the component isn't yet mounted, because the render pass in which it is created has not yet finished.

In React 18, in strict mode during development, it will call render() on your components twice in order to, amongst other things, help to detect such issues (e.g. side-effects during rendering, which are dangerous in React 18 when concurrent rendering is used). Passing a state setter to "outside of React", e.g. as a callback for something else, is such a side-effect that's potentially dangerous.

[aleksandrlat]

People having a lot of troubles with this in my company too
We see the same operation is resolved before useEffect in src/useForceUpdate.ts called so component does not re-render and displays old information

https://github.com/relay-tools/relay-hooks/pull/241/files#diff-6db1059a1fc685c06d6242d40d78666919ee58be77913785966804264c54f88cR6-R11

[morrys]

released with relay-hooks v9.0.0

[navyblue21]

I have same issue for iOS v14.4, React 18.2.0