mobx: [Bug] Error: Maximum update depth exceeded

If it is decided to keep the global state version increase on observable creation

It’s a bug, the plan is to solve it on mobx side, just can’t provide an estimate.

I am not a Next.js expert but the useState(new Store()) is wrong because it recreates store on each component render. useState(() => new Store()) should be used instead

You can track the progress here https://github.com/mobxjs/mobx/pull/3732

@kubk That should not matter, we’re not calling setState which means that the component should not re-render, which implies that the instance is not created indefinitely. I did edit the description downgrading mobx-react-lite to 3.4.3 and removing observer also fixes the issue.

We partially reverted the behavior for now in mobx-react-lite@4.0.5. Although very strictly speaking the pattern is behaving as it should, we can still support the old behavior without changing the other semantics. This might change in the far future if we ever want to support tear-free currency support but for now that pattern should work again as it did before.

@mweststrate doesn’t really work for my use case because I want to update the observable every time a property is updated. Assuming I go extra mile and I track dependencies it would still run the effect since the element prop would get a new array instance every time.

I guess my real question is why an update to an observable local to a component triggers a re-render of the parent component?

@jraoult Could you give an example of your implementation?

I think having lazy state initialization in useState should do the work, because react only create an instance of the store once. (no need for useMemo).

The problem is that makeAutoObservable (described by @urugator) counts as mutation which means the observer higher in the tree is detecting changes, which causes re-render, then nonlazy useState initialization is creating a new instance of the store (with each re-render, calling makeAutoObservable again), which causes another mobx mutation and the entire process is repeated again.

@benz2012 we don’t do global version numbers anymore, so likely you’re hitting a very different issue with just the symptom; any recursion problem in JS will appear as Error: Maximum update depth exceeded.. So yeah best open a separate issue. Although you probably already will find the root cause by isolating it; typically either hope that break-on-error in your debugger pinpoints it, or binary-search disabling half of your code base until you isolate the problem is the way to go for these.

Particularly to the last point, it seems the global version number totally breaks fine-grained reactivity?

Again, note that this whole problem only exists during the initial mount of the tree, not thereafter! So the fine grained reactivity is still working as it should. That is why stop shoving referentially speaking new data in when you want to render the same thing stops the problem.

We’ll investigate further as said, that will take a few weeks probably due to personal plans. If you’re blocked for now the easiest thing is probably to downgrade MobX package, as that will cause mobx-react-lite to fallback to local versioning id’s.

On Thu, 14 Sept 2023, 07:33 Stephen Haberman, @.***> wrote:

you must never see an output where coords.x lags behind coords.y or vice versa

This is a good point, that we must keep this lagging in mind–but, afaiu & as you said, mbox fires reactions immediately/sync.

A series of mutations to different atoms, i.e. “a.x += 1”, “a.y += 2”, “a.x += 3”, should, afaiu, never interweave in a way that “atom a’s reactions were fired & Component A’s onStoreChanged+rendered” while “atom b’s reactions were totally skipped & it’s Component B is stale” (i.e. tearing), even without a global version number.

1. The callback used to notify component is normally called if ANY part of the store changes

If you mean “in canonical useSES usage, the React examples all call the storeOnChange method on any change to their global store”–that makes sense, but I don’t think Mobx must do that, just like we’re already creative/non-canonical by returning a version number from getSnapshot and not “the snapshot”.

1. In Mobx, you don’t know what the component depends on, until you actually run the render function

True; maybe you’re thinking we could “miss state changes” that happened before our render was called?

Afaiu that can’t happen again b/c Mobx is sync…when the render function runs, it must see the latest state (so nothing torn), and the render function + useObserver dependency tracking will happen atomically, so we can’t miss an update (become torn pre-commit).

1. Normally the state (snapshot) [returned from useSES.getSnapshot] is immutable -[…] In mobx you can pass a reference […] that surely won’t be communicated [trigger] via props/contex

You’re again right–store mutations won’t be communicated via props/context, but they will be communicated via reactions to useSES’s onStoreChange + local version bump (only to the components that are actually watching the changed atoms, which is exactly how mobx is supposed to work).

Particularly to the last point, it seems the global version number totally breaks fine-grained reactivity? If every component with useObserver + useSES on a page gets a global version/snapshot bump on every mutation, how is Mobx’s fine-grained reactivity not completely broken? 🤔

I dunno, you’ve definitely got the hard-won expertise here from fighting/solving things like the strict mode/double rendering/mounting protection (❤️ ), but I wonder if maybe we’re being too pessimistic wrt to useSES’s semantics, and trying too hard to “act like an immutable global store b/c that’s what the React team lives & breathes and built useSES around”.

At the very least, I thought when I asked my naive question of “…why do we need a global version number”, that there’d be ~2-3 rock solid “gotchas” that I’d completely missed/didn’t know about, but so far it sounds like not (?), and it’s more out of caution and useSES’s overall opaque behavior.

— Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/3728#issuecomment-1718789230, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBD22AJHPZAH3VKUAALX2KJJXANCNFSM6AAAAAA2N5BIB4 . You are receiving this because you were mentioned.Message ID: @.***>

Valtio has basically the same problem: https://stackblitz.com/edit/stackblitz-starters-n5y1zc?file=src%2FApp.tsx

The difference is that in valtio, you can have multiple independent stores. But afaik state in valtio is actually immutable and it always invalidates the component that uses useSnapshot - you can’t pass this snapshot to a child components and “subscribe” to it individually - the state/snapshot is truly local for that component and the only way to propagate it to other components is via props/context.

I know I just posted a long comment, but I’ll try to very succinctly +1/fwiw a few snippets from the thread:

As @jraoult said:

I guess my real question is why an update to an observable local to a component triggers a re-render of the parent component?

Yep! This is my question/confusion as well.

In @urugator 's assertion that “all of mobx is a global store anyway”:

All mobx observables are treated as a single external state/store from React’s perspective […] Since we can’t split our state into isolated bits, that are known upfront, the way we approach this is that all observables

Honestly I don’t know the internal Mobx implementation details pre-useSES, but personally my mental model was that each observable “sub-graph” / “sub-tree” of data was its own “island”, b/c if I mutated data in observable/sub-graph A (say the child’s internal observable), that was 100% disconnected from observable/sub-graph B (a separate observable the parent is watching), then no components watching sub-graph B would ever re-render.

Granted, I get it’s tricky b/c the graphs of observables/atoms/etc morph over time, so sub-graph A & sub-graph B could become tangled/untangled as a computed’s dependencies change.

But, I dunno, I’m just saying that somehow pre-useSES mobx supported/placated my “separate sub-graphs” mental model very well, and now the useSES approach has pretty fundamentally changed it.

this makes observables fundamentally non local.

True, but that doesn’t necessarily mean global. It seems like previously mobx could thread the needle on “not local but also not global” reactivity…

Given the “sub-graph A vs. sub-graph B” mental model is hard to actually implement (dynamically tracking graph connectedness), I guess I thought that each mobx observer/reaction just watched the atoms/signals it had directly/immediately read during render/computation, and would only re-render when those changed, which effectively achieves the “separate sub-graphs” mental model.

I get that useSES’s own mental model really wants/assumes “a global store”, but can mobx’s implementation of its getSnapshot still return basically “one of my immediately watched atoms got bumped since last render” to communicate “I’m torn”, without relying on a global version number?

@urugator

The fix is to make sure you don’t mutate state on every update - either by using deps array or explicit check:

Fair enough, but I guess for my complete use case (I spare the details) it is easier to “stabilise” the array before passing it to the sub component and therefore avoiding re-render, than checking inside the effect for equality 🤷‍♂️.

@jraoult

The fix is making the value passed though element prop stable with useMemo

Not really. The fix is to make sure you don’t mutate state on every update - either by using deps array or explicit check:

useEffect(
  action(() => {
    if (!shallowEqual(store, props)) {
      Object.assign(store, props);
    }      
  })
);

@mweststrate

no observable is read in it, so why does it rerender? Feels like an incorrect handling of nested tracked sections.

It has nothing to do with tracking, it’s because of how useSyncExternalStore works. In getSnapshot you’re supposed to return snapshot - a stable part of your state that you’re going to use inside your component. Since we can’t split our state into isolated bits, that are known upfront, the way we approach this is that all observables are part of a single “snapshot”, so we replace all observables by a single stable value - state version. It’s like having a single immutable store, every component depends on this store and you don’t use any selectors. Check this: https://stackblitz.com/edit/stackblitz-starters-abrrac?file=src%2FApp.tsx,src%2Findex.tsx Now you might be saying, that’s horrible what about fine grained reativity. You have to realize that getSnapshot isn’t a mechanism for invalidating components. React calls getSnapshot on a component that is already being updated or when unsuspending a component, not being sure whether the output is still valid (the output is invalid if any part of the external store changed - reacts internal state is de-synced with external world). One other case it has to check getSnapshot is during mount, which is the situation we are dealing with here. Do you remember how we used to check whether the reaction was invalidated inbetween first render and useEffect and if it was, we had to immediately schedule re-render? Well that’s exactly what React is doing here. The subscription function passed to useSyncExternalStore isn’t called until the component is being commited. So React has to check whether external store didn’t change between the initial render and commit. So getSnapshot is called and we return different snapshot (version) than on initial render, so it restarts rendering and so it goes in cycles.

I think I want to dig into this one a bit deeper. Some random thoughts:

1. An unconditional useEffect that always mutates resulting the given error doesn’t sound like a bug at the conceptual level to me.
2. However, the Parent App rerendering of App purely due to the marking with observer does look like a bug (removing it solves the problem), no observable is read in it, so why does it rerender? Feels like an incorrect handling of nested tracked sections.

https://stackblitz.com/edit/stackblitz-starters-jxldkn?file=src%2FApp.tsx

I think I might be running into a similar problem with upgrading to mobx-react v9.0.0. The symptom is the same where I am getting a “Maximum update depth exceeded” error but the case is slightly different. My app uses a universal store and then some subcomponents use a separate store.

Example here: https://codesandbox.io/s/minimal-observer-forked-gvf827?file=/src/index.tsx

Downgrading back down to mob-react v7.6.0, I can click the Browse and Edit buttons to toggle back and forth between pages and they render fine. But with the latest update, clicking Edit which uses a ParentStore AND a ChildStore will result in error.

@MateuszLeo yes, using useState if I don’t need to track dependencies (or useMemo otherwise) works, but then I need to rewrite all my accesses everywhere.

So here’s a simplified example just for the sake of demonstration, but it is what I have in my code base by dozens:

class MyObservable {
 constructor($model) {
   makeAutoObservable(this);
  }
  [........]
}

class MyFacade {
  constructor($model) {
     this.$model;
     makeAutoObservable(this);
  }

  get myLazy() {
    return new MyObservable(this.$model);
  }
}

and then

const MyCmp = observer((props: {myFacade: MyFacade}) => {
  return <SubCmp myObservale={props.myLazy}/>
})

This now fails and needs to be rewritten:

const MyCmp = observer((props: {myFacade: MyFacade}) => {
  const = [notLazy] = useState(() => props.myLazy);

  return <SubCmp myObservale={props.myLazy}/>
})

This is doable, although cumbersome, but you can imagine it is more complicated in real life. I have conditional access and branches. Then the hook-based solution shows its limits quickly since they can not be called inside conditions etc.

Quick thought: should we avoid increasing global state version for non-observed atoms?