nanostores / query

⚡️ Powerful data fetching library for Nano Stores. TS/JS. Framework agnostic.
MIT License
228 stars 10 forks source link

`Cannot update a component while rendering a different component` Error When Using Fetcher In Multiple Places #33

Open martinmckenna opened 8 months ago

martinmckenna commented 8 months ago

Simplified reproducible example: https://codesandbox.io/p/sandbox/nanostoresreact-test-3wzp6z

To test:

  1. Open example codesandbox
  2. Click on select dropdown and select option
  3. See Cannot update a component while rendering a different component console error
Screenshot 2024-02-26 at 5 33 29 PM

I'm having an issue where I'm trying to follow the example from here in the README regarding sending parameters down, basically exactly as the docs recommend: https://github.com/nanostores/query?tab=readme-ov-file#local-state-and-pagination

The issue I'm having is when attempting to re-use the same fetcher in another component with the same query key and arguments, I get this error.

My actual use-case is as follows

  1. Load page. First useStore(store) runs
  2. Open pop-out drawer to add a new entity, which also needs to call the same useStore(store)
  3. Error appears because now 2 components are mounted that are calling the same nanostore

I've done some digging and it seems to be something related to useSyncExternalStore. Another data fetching library had a similar issue, which was documented here:

https://github.com/urql-graphql/urql/issues/1382

They ended up just not using useSyncExternalStore, but I'm not 100% sure this issue is a problem with @nanostores/react because I tried my best to replicate without even using @nanostores/query and it didn't seem possible, which is why I'm logging it here.

There is also a few relevant react issues filed around this error, but they don't seem to be very helpful:

https://github.com/facebook/react/issues/26962 https://github.com/facebook/react/issues/18178

dkzlv commented 8 months ago

Much like the "The result of getSnapshot should be cached to avoid an infinite loop". It won't even be reported in Sentry or other error tracking software if you make a production build. As for bugs/performance, that's an open question.

getSnapshot should be cached

This is the StrictMode issue. It calls all the effects twice in dev mode (including the constructor function of useState), and it expects that the result of calls in useSyncExternalStore will be memoized by an outside entity—including the identity. We use objects (e.g., {loading: true}), and we don't want to memoize them, because there's actually no benefit to that. It sees that there are two different objects in two separate calls—it warns you, because if you had side-effects (say, an atom that has a random value on each mount) it could potentially break the concurrent mode. We have an external storage that pushes the values into stores, so this warning is, unfortunately, a false positive. I'm not entirely sure how to fix that without writing a lot of react-specific code into the core.

This will definitely not hurt the performance or result in any bugs whatsoever.

Cannot update a component (Parent) while rendering a different component (Child)

This is a nanostores issue, probably, but again, that's not an issue at all. Due to the fact that we're framework agnostic, we're not tailoring the code to React's expectations of scheduling effects. So you subscribe to the same store in two components, and those two are in dependency of each other. To fix this issue we would need to redo the whole scheduling system we have and call subscribers layer by layer, like child → parent instead of the way we do it now, which is in the order of subscription.

As for performance/bugs here, I'm not sure. I think there shouldn't be any substantial performance problems (maybe an extra render?). As for bugs, there's a possibility (I never tested it actually) of stale state in child components, so if any effects run in-between renders they may end up using old values.

I'll ask around about this.