[Bug] Error: Maximum update depth exceeded

[MateuszLeo]

Intended outcome: Render without any issues

Actual outcome: Error: Maximum update depth exceeded.

How to reproduce the issue: Repro:

• Next.js https://stackblitz.com/edit/stackblitz-starters-wpvmwt?file=pages%2F_app.js
• Pure react https://stackblitz.com/edit/stackblitz-starters-aa5t8d?file=src%2Findex.tsx

1. _app.jsx - define the component as observer
2. index.jsx - use useState to store mobx store without lazy initialization (useState(new Store()))
3. App crashes because of Maximum update depth exceeded

Any component defined as observer higher in react tree than the defined mobx store is causing the issue above.

Lazy initialization (useState(() => new Store())), downgrading mobx-react-lite to 3.4.3 fixses issue or removing observer fixes issue.

Versions mobx-react-lite@4.0.3

[kubk]

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

[MateuszLeo]

@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.

[urugator]

The issue is that observable initialization (makeAutoObservable, ...) also counts as a mutation, increasing state version. As a workround don't create observables during render. Dunno what would the best way to handle this atm.

[mweststrate]

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

[urugator]

Imo no, basically for the same reason as noted here https://github.com/mobxjs/mobx/issues/2526#issuecomment-1248238184

[jraoult]

I've been spending a fair amount of time the last two days trying to migrate and figuring out a solution.

I understand it is not idiomatic anymore, but I have a five-year-old code base that relies on a getter to lazily-create observable stores. Wrapping all these getter access with useMemo would be extremely daunting, but also, because of the rules of the hooks (no conditional etc.) will require massively reorganising the code and will change the semantics in some parts

If it is decided to keep the global state version increase on observable creation, is there a solution that doesn't require using hooks? I'm thinking of something with an ergonomic equivalent of action() but that would stop the version increase.

[MateuszLeo]

@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.

[jraoult]

@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.

[urugator]

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.

[maccman]

We just ran into this and had to revert too.

[urugator]

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

[jessetruong]

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]

@jessetruong I think cases are pretty much the same as you're also creating new instance of ChildStore during render, the only difference is that your not using useState to store it

[dnish]

We've the same issue with using a third party swipe component. When we upgrade mobx-react to version 8 or 9, the app breaks with the message above. Reverting back to 7.6.0 fixes the issue.

As you mentioned, the issue is caused by creating a store within the component? We are using the "official" way with useLocalObservable.

[geakstr]

We've hit the same issue and wasn't able to find what component actually causes this, rollback to 7.6.0 helps.

[gregg-cbs]

On nextjs i found that I was missing mobx's enableStaticRendering

// some component
import { observer, enableStaticRendering } from "mobx-react-lite"

enableStaticRendering(typeof window === "undefined")

export default observer(()=> {
  const [someClass] = useState(()=> new mobxClass())

  ... bla bla bla
})

This fixed the maximum depth error in nextjs.

[jraoult]

@urugator so I finally managed to try this bug fix. I still think there is an issue. Something like that was working before (in 6.9.0) and is now (6.10.2) triggering a Maximum update depth exceeded error:

import { action } from "mobx";
import { observer, useLocalObservable } from "mobx-react";
import { useEffect } from "react";

const createModel = () => ({
  element: [] as Array<unknown>,
});

const Sub = observer((props: { element: Array<unknown> }) => {
  const store = useLocalObservable(createModel);

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

  return <h2>Placeholder</h2>;
});

export const App = observer(() => (
  <>
    <h1>Hello sandbox 👋</h1>
    <Sub element={[]} />
  </>
));

Somehow updating the local observable in the Sub component, triggers the App re-render which creates a new instance passed to the element prop which triggers a re-render of Sub then run the effect which updates the local observable etc.

Should I create a new issue or is it the expected behaviour?

[mweststrate]

UseEffect should have deps at least?

On Wed, 6 Sept 2023, 19:09 Jonathan Raoult, @.***> wrote:

@urugator https://github.com/urugator so I finally managed to try this bug fix. I still think there is an issue. Something like that was working before (in 6.9.0) and is now (6.10.2) triggering a Maximum update depth exceeded error:

import { action } from "mobx";import { observer, useLocalObservable } from "mobx-react";import { useEffect } from "react"; const createModel = () => ({ element: [] as Array,}); const Sub = observer((props: { element: Array }) => { const store = useLocalObservable(createModel);

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

return

Placeholder

;}); export const App = observer(() => ( <>

Hello sandbox 👋

<Sub element={[]} />

</>));

Somehow updating the local observable in the Sub component, triggers the App re-render which creates a new instance passed to the element prop which triggers a re-render of Sub then run the effect which updates the local observable etc.

Should I create a new issue or is it the expected behaviour?

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

[jraoult]

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

[urugator]

Should I create a new issue or is it the expected behaviour?

In this specific example, you get Warning: Maximum update depth exceeded even with previous mobx/react versions - whether parent is rendered or not is not relevant: https://codesandbox.io/s/delete-me-y5hr9p?file=/index.js

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

It's not really "local" in terms of some kind of isolation. It's just an observable created on mount and held in useState. All mobx observables are treated as a single external state/store from React's perspective. You can send a reference to this "local" observable to other components, they can subscribe to this observable and can be individually notified about it's changes, without the need to invalide the original source component first and propagate the change through props/context - this makes observables fundamentally non local. From what I could observe the current behavior is this: All observer components mounted together with the component that updates any observable synchronously during use(Layout)Effect, will be immediately re-rendered as well. Observers that were already mounted are unaffected by the effect. You can check it here: https://codesandbox.io/s/delete-me-2-4cvc4c?file=/index.js Why this has to be the case I dunno, but I don't think we will be able to do anything about it with the current approach.

[jraoult]

@urugator many thanks for your reply and the explanation of the logic behind that said my question remains:

in this specific example, you get Warning: Maximum update depth exceeded even with previous mobx/react versions

In this specific example indeed but it is a modified version of what I provided where the maximum depth error make sense. But it is different that the reduction I provided. I insist that for my use case this is most likely a regression since it was working before 6.9.0 (cf. https://stackblitz.com/edit/stackblitz-starters-f9zpi8?file=src%2FApp.tsx with Mobx 6.8.0) and stopped after that (cf. https://stackblitz.com/edit/stackblitz-starters-tvchp3?file=src%2FApp.tsx with Mobx 6.10.2) and this is because the parent component re-renders where it wasn't before.

The fix is making the value passed though element prop stable with useMemo but I guess I just want to know if it is a regression or a feature/bug fix. In the former I'll wait for a fix version but in the latter I'll just have to fix my code (but then may be it should have been a major version bump for Mobx).

[mweststrate]

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

[urugator]

@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.

[jraoult]

@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 🤷‍♂️.

[urugator]

@jraoult Whatever works best for you, just keep in mind that useMemo does not have this "stabilization" as a semantic guarantee:

You may rely on useMemo as a performance optimization, not as a semantic guarantee. In the future, React may choose to “forget” some previously memoized values and recalculate them on next render, e.g. to free memory for offscreen components. Write your code so that it still works without useMemo — and then add it to optimize performance.

Also notice what useMemo is - a local state updated every render under some condition.

[stephenh]

Fwiw I just started seeing the same behavior as @jraoult ...

Everything works fine w/mobx 6.10.2, mobx-react 7.6.0, & React 18.2, but if I bump only mobx-react to 9.0.1 then I see:

1. A standard/boring MyPage functional component that does const rows = [a, b, c] (not memoed) & passes it to <GridTable rows={rows} ... />, then
2. Internally GridTable uses a runInAction within a useEffect (with deps on props.rows) to push the props.rows into its internal mobx-based store
3. Very oddly, this causes MyPage itself to re-render (I believe b/c it is watching a separate observable with a <Observer>{() => ... })</Observer>), which re-creates a new rows, which re-renders GridTable with a new props.rows, which reruns the runInAction, which causes MyPage to re-render...
4. Infinite loop

I can work on a smaller reproduction if desired, but thought I'd type out the tldr just b/c of how eerily similar it is to @jraoult 's description + already small reproduction. Feel free to ignore this b/c it's not "a small repro", but the table component mentioned above is actually open source, so you can see the runInAction snippet here:

https://github.com/homebound-team/beam/blob/main/src/components/Table/GridTable.tsx#L243

Per the in-source comment on that line ~240, afaiu "pushing props into an observer from a useEffect" is actually the most/only kosher way to cross the "incoming React/useState-managed props ==> mobx-managed observable" divide without causing "render of component A caused update of component B" errors (if observables are directly updated during the render/not from the useEffect).

So to have it break/infinite loop seems really surprising...

If I'm following @urugator 's assertions (still reading), I believe he'll assert that either a) our MyPage should be responsible for useMemo-ing its rows before passing the prop, or b) our GridTable.setRows should shallow/deep compare the rows to realize they are the same...

This is a fair assertion, and our performance-sensitive large pages/tables actually do this--however, I assert the React mental model is that useMemo usage is supposed to be optional/opt-in, only on a as-needed/wanted basis, and so the current mobx-react 9.x behavior of "make sure you useMemo whatever data/props might later cause an observable to be created or you'll infinite loop" seems like a large regression.

(It also just seems odd that a child component is causing my parent component to re-render, when the parent is not observing anything about the child's internal state/observers ... granted, the parent does happen to have it's own separate observable, which is probably what is triggering the ping-ponging of child/parent/child/parent re-rendering each other... I get @urugator 's explanation that useSES nuances mean that "all mobx observers are now globally linked / version bumped", but fwiw I thought that mobx observables could be non-global/non-redux-ish stores was one of the great things about the framework. The move to useSES seems to have broken that behavior.)

[stephenh]

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?

[jraoult]

@stephenh Not sure it is relevant, but I also crafted a version of my reduction but using Valtio instead (that is fundamentally based on useSES). It doesn't exhibit this global re-rendering on mutation and therefore works like Mobx pre-6.9.0. So I wonder if the behaviour described by @urugator in Mobx 6.10.2 is because there is not way in Mobx-react to get a local "snapshot" for reads whereas Valtio requires it and that therefore the whole tree needs to be re-rendered?

[mweststrate]

Thanks for reviving the context again here @urugator

Thanks for the feedback folks. We need to investigate this one further to see if we can somehow better square this circle.

To recap (as most has been said already above): Note that the updates, after the first rendering has been completed, will be local as you are used to from MobX. The underlying React fundamental we are running into here is that React want's to eliminate so called "tearing": The tree being rendered with two different global versions of the world at the same time. So since the child component keeps changing state during render, it keeps starting over and over again with rendering and it never finishes.

So the breaking change here is indeed to be a "proper" react citizen, and play along with the tear-free rendering, which MobX so far didn't do / care about (and React neither before concurrent rendering).

The obvious way to fix that here is to stop generating new "state" as part of the render, e.g. in the App comp, make the empty array a global constant, or store that empty array in useState, is an elegant and conceptual correct solution. It also avoids the diffing need in Sub (as the rendering now stabilizes after the first effect run; App will run a second time, but not cause a new effect in Sub).

const EMPTY_ARRAY = [];

export const App = observer(() => {
  console.log('APP');
  return (
    <>
      <h1>Hello sandbox 👋</h1>
      <Sub element={EMPTY_ARRAY} />
    </>
  );
});

Or

export const App = observer(() => {
  console.log('APP');
  const [initial] = useState([])
  return (
    <>
      <h1>Hello sandbox 👋</h1>
      <Sub element={initial} />
    </>
  );
});

Note that making the effect async (e.g. something like Promise.resolve().then(() => { Object.assign(store,props) }), also fixes the problem, as then the tree has been committed already, and the state is mutated after render, so a 2nd tree can be expressed as an update to the first. However this might introduce a visual flash and the above solution is the conceptually correct one.

I'm not sure how Valtio differs, if someone wants to make a sandbox happy to look, but I suspect it might be because most other reactive frameworks propagate changes asynchronous in a microtask, so likely it is effectively the Promise solution mentioned above, but baked into the framework.

[jraoult]

if someone wants to make a sandbox happy to look,

@mweststrate there you go: same thing but with valtio 😉

[mweststrate]

at the risk of being lazy: mind adding a store read to App and Sub :) (can be an read of something else than elements to make the case similar) ?

[jraoult]

@mweststrate will do but just to reiterate, my initial point was exactly about a re-render that happens even if I don't read anything, just write (cf. https://github.com/mobxjs/mobx/issues/3728#issuecomment-1708783622).

[mweststrate]

yeah I'm aware, but there is observer on App in the example (removing it fixes the problem as well), but I assume we conceptually we want to have that there, as some other observable might be read in it :)

[urugator]

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.

[jraoult]

@mweststrate I went to update my code but I found a edit from @urugator (I didn't know it was possible for someone else to edit my sandbox 😀). That said I disagree with your update @urugator. I think a more accurate version of what I'm trying to do is to call useSnapshot on another proxied object that is not the one updated in the sub component:

import { useEffect } from 'react';
import { proxy, useSnapshot } from 'valtio';

const otherStore = proxy({ somethingelese: '' });
const store = proxy({ elements: [] });

export const App = () => {
  // This is analogous to using `observer` without actually accessing any observables
  // (EDIT by jraoult)
  const sn = useSnapshot(otherStore);
  // App does NOT subscribe to sn.elements
  return (
    <div>
      <h1>Hello</h1>
      <Sub elements={[]} />
    </div>
  );
};

const Sub = (props: { elements: Array<unknown> }) => {
  useEffect(() => {
    console.log('Effect run');
    Object.assign(store, props);
  });

  const sn = useSnapshot(store);
  // Sub DOES subscribe to sn.elements
  sn.elements;
  return <h2>Placeholder</h2>;
};

[jraoult]

Note that making the effect async (e.g. something like Promise.resolve().then(() => { Object.assign(store,props) }),

@mweststrate brilliant, that might be my way forward then 👏

[stephenh]

@mweststrate (and @urugator / @jraoult ) really appreciate the time & reply!

So the breaking change here is indeed to be a "proper" react citizen

I'd like to push back that on--in my example, neither the parent nor child were going to tear, b/c they were watching two completely stores/atoms.

It is only this (false/incorrect imo) sense of "all mobx observables are now a single global store" that means Mobx's current useSES has this regressive/infinite loop behavior, b/c it is pre-preemptively re-rendering any staged-to-commit component whenever any atom changes, regardless of whether that atom would actually invalidate the component's previously-rendered output.

Which seems unfortunate (crazy?), b/c afaiu the purpose/power of mobx's per-atom dependency tracking is precisely so that mutations can trigger reactions for only their immediate/current downstream dependencies, and not do wasteful "re-render everything just in case" type approaches.

Here is an example where tearing would never happen w/the old mobx behavior (in terms of inconsistent state being shown in the same DOM snapshot), but it currently infinite loops:

import { observable } from "mobx";
import { observer } from "mobx-react";
import { render } from "@testing-library/react";
import { useEffect, useMemo } from "react";

const storeOne = observable({ name: "John" });

export const Parent = observer(() => {
  // this infinite loops
  const rows = [1, 2, 3];
  // but works fine if you useMemo it
  // const rows = useMemo(() => [1, 2, 3], []);
  return (
    <div>
      {storeOne.name} <Table rows={rows} />
    </div>
  );
});

export const Table = observer(({ rows }: { rows: number[] }) => {
  const tableStore = useMemo(() => observable({ rows: [] as number[] }), []);
  useEffect(() => {
    tableStore.rows = rows;
  }, [tableStore, rows]);
  return <div>{tableStore.rows.length}</div>;
});

describe("Render", () => {
  it("should render", () => {
    render(<Parent />);
  });
});

The parent & child have completely separate observables, but if the parent passes a new array (which is perfectly valid React code) into its child, which happens to also store it into an observable, now our two component ping/pong each other to death.

I guess what I'm not following is why Mobx's useSES impl has to defacto use "a global store counter"--I get that "external-probably-global stores" is what the React team built useSES for (for the Reduxes or Apollos or etc of the world), but, in Mobx, can't each component have it's own local version, it's own local "snapshot", and that counter is ticked only when one of its reactions run?

Afaiu this local counter, with useSES, still solves tearing--if a component renders, but is async interrupted, then one of its dependent atoms changes (the reaction runs), that would tick its useSES counter, and it would re-render without tearing.

(Fwiw this is what legend-state's useSelector does, which is like their useObserver, and their trackSelector is like mobx's reaction.)

Where's the need for a global counter?

IANAE on mobx internals, but the only thing I can think of is that, afaiu, there are times when (maybe?) reactions cannot immediately be ran (or maybe it's just computeds?), so they're like queued a little bit, and maybe these extreme (in my day-to-day usage anyway) boundary cases is where the reaction itself can't be relied on to always be ran before useSES's getSnapshot is called, and so we have to pre-emptively say "any mutation to any observable --> immediately bump every component's useSES snapshot". ...maybe?...I hope not, but just trying to understand what the hard requirement on the current approach is.

[urugator]

but, in Mobx, can't each component have it's own local version, it's own local "snapshot", and that counter is ticked only when one of its reactions run?

We actually do that if globalVersion isn't available: https://github.com/mobxjs/mobx/blob/91adf79619533da4de04737eae83c61a4a8ffb92/packages/mobx-react-lite/src/useObserver.ts#L16-L20 There is one caveat though - globalVersion is intentionally updated on mutation, while this local state version is updated via reaction. We need a guarantee that react won't have a chance to call getSnapshot before the reaction is invoked - just something to be considered.

neither the parent nor child were going to tear, b/c they were watching two completely stores/atoms.

Firstly I want to say I am not completely sure about this and it could be the case that local state version is really sufficient. Unfortunately due to the amount of possibilities and the concurrency factor, it's quite hard to contemplate about this and almost impossible to write a rigor test or proof. Unless you know React internals (I don't) you have to do a lot of guessing and trying. I assume/think that what matters isn't only whether 2 components depends on the same the atom, but whether these atoms are correlated in such a way, that their snapshots/versions taken in different moments in time (a moment when render fn was called), must not ever be displayed to a user at the same time. Eg you have a coordinates {x,y}, a component X displays coords.x, component Y displays coords.y, each component depends on different atoms, but you must never see an output where coords.x lags behind coords.y or vice versa. Notice it can get a lot more complicated, because you can introduce intermediate component, that isn't even observer, but recieves coors.x as props/context. It can then eg move this coors.x to local state, calculate some value from it, send it further, use it as a condition etc etc. Once you output all these components to screen, you must see a result that is consistent. So you have to somehow tell react, these outputs might actually be somehow related. Mobx is a "sync" store, so you can assume that if there is a correlation between coords.x and coords.y, they will be updated at the same moment - it's physically impossible for the external system (React) to see them desynced. Normally in useSES the getSnapshot can return 2 different parts of the same store (coords.x/coord.y) in two different components and still prevent tearing, so why mobx needs to synchronize different components using the same "snapshot"? Well, as I pointed out earlier, maybe it doesn't, but there are some differences in mobx you have to take into consideration:

1. The callback used to notify component is normally called if ANY part of the store changes. So all components depending on the same store are notified. They can prevent update based on the snapshots equality. In mobx this callback is invoked only if one of the observed observable changed. If the callback is invoked, the snapshot must differ.
2. Normally the state (snapshot) is known upfront - the component knows that next render can't depend on anything else than the thing returned from getSnapshot. In Mobx, you don't know what the component depends on, until you actually run the render function. There is also no "entry point" for the state. You can simply import any observable and use it. There seems to be an entry point when using useLocalObservable, but all observables are treated equally, they don't have any locality.
3. Normally the state (snapshot) is immutable - this means that if you want to propagate the state to a different component, the component will be surely invalidated due to recieving new props/context. In mobx you can pass a reference, so there is a possible relation between parent and child output, that surely won't be communicated via props/context (if there is a memo).

It would be suprising to me, if you wouldn't need a mechanism to compensate for these, but it's a possibility. That being said impl with local version was passing these https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-rendering which was a bit suprising as well.

which is perfectly valid React code

It's valid, it's not idiomatic: https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes

My guess is, that if you wouldn't use Mobx, you wouldn't even move the prop to local state. I think you use tableStore.rows only as a convinient way of drilling the props.rows further, but tableStore.rows itself isn't really a state (something you could mutate), because it must always reflect the value in props (unless it's used only for init, in which case it should run only on init and not with each new props.rows).

[stephenh]

I think you use tableStore.rows only as a convinient way of drilling the props.rows further

100%. We have a complicated Table component within which we want to use Mobx's robust, amazing incremental re-rendering/computed caching/etc. capabilities...while still providing a regular React/props-based API to external callers.

but tableStore.rows itself isn't really a state

In this simplified example, you're right, but in the real component we use props.rows to seed RowState models that then track & mutate our internal per-row state (collapsed yes/no, filtered yes/no, etc). So, it's not "just drop the props.rows into store.rows", but "when props change, do a transformation that effectively means 'update (part) of the store'".

Fwiw I would have assumed this "use an internal mobx store to implement complicated React components" approach would be a pretty normal thing to do? :shrug:

It's valid, it's not idiomatic:

If you're saying "don't adjust state [update the observable] via a useEffect", I would love to not do this, don't we have to, to avoid "Component A cannot update state in Component B" warnings?

I.e. previously, if Component A accepts props and immediately updates its internally-managed observable store, and that store mutation causes a sync reaction to a useObserver that setState(tick)s in Component B --> that is a React warning (which AFAIU generally plagues mobx users, but you can avoid by only pushing props --> observables in useEffects).

Kinda wonder, given useSES is based on "polling for snapshot changes", does this mean we are not doing setState(tick) anymore? So we could push props into observables directly from render?

...actually, that makes it even worse, in mobx-react 9.x:

export const Parent = observer(function Parent() {
  // note I'm using the "safe" useMemo-d array
  const rows = useMemo(() => [1, 2, 3], []);
  return (
    <div>
      {storeOne.name} <Table rows={rows} />
    </div>
  );
});

export const Table = observer(function Table({ rows }: { rows: number[] }) {
  const tableStore = useMemo(() => observable({ rows: [] as number[] }), []);
  tableStore.rows = rows;
  return <div>{tableStore.rows.length}</div>;
});

Both infinite loops and warns that "cannot update Table while rendering a different component Table"`:

  console.error
    Warning: Cannot update a component (`Table`) while rendering a different component (`Table`). To locate the bad setState() call inside `Table`, follow the stack trace as described in https://reactjs.org/link/setstate-in-render
        at Table (/home/stephen/homebound/internal-frontend/node_modules/mobx-react-lite/src/observer.ts:104:54)
        at div

So I guess I'll have to stay with "map props -> stores from a useEffect" for now...

Fwiw if you look at legend-state's useSelector, the way they use useSES does avoid the infamous "cannot update during render" error messages, b/c they just 'tick' a local let version variable (no setStates :100: ) that gets returned from the getSnapshot to let useSES knowing a re-render is necessary. (I.e. the point of useSES is to replace useStates for triggering re-renders, not augment it...)

Could mobx just do what legend-state is doing? No global version number, and no setStates...

I haven't had a chance to fork useObserver and try it myself, but I'll try to get to it soon.

For your points, @urugator , about { x, y } are different atoms, so "x => Component A", "b => Component B" means "we're torn"...personally I don't think so, but you might be right, I'll need to think about it...

Fwiw/either way, I think we can all agree that while useSES's API seems relatively simple & straightforward at first, but actually reasoning about its behavior & implementation is surprisingly difficult and at times we're left to just guessing. :-/

[stephenh]

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".

2. 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).

3. 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? :thinking:

I dunno, you've definitely got the hard-won expertise here from fighting/solving things like the strict mode/double rendering/mounting protection (:heart: ), 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.

[mweststrate]

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: @.***>

[mweststrate]

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.

[stephenh]

Awesome! Thank you @mweststrate !

in the far future if we ever want to support tear-free currency support

Fwiw/AFAIU the current implementation (where you removed the global version check) still provides tear-free concurrency support...

I.e. if we render x.color, and x.color changes before DOM commit, the component's x.color reaction will bump the local version, and useSES will de-opts/re-render with the latest x.color.

The :thinking: / debate AFAIU is whether a separate, non-dependent property (potentially in a completely separate observable/completely separate store) like x.size (or y.size if a separate store) changing should lead to x.color being considered torn.

Imo it shouldn't, b/c if x.color depended on x.size or y.size, we'd know about it, b/c it would be encoded as a computed/dependency in the mobx dependency graph.

...but maybe this is just a agree-to-disagree about what "tearing" means. I get that "tearing" to most global stores/in colloquial useSES usage means "there as been any new write to any part of the global store", but in theory mobx is uniquely capable of being smarter than that. :shrug:

Either way, really appreciate your work! Thank you!

[benz2012]

Is this issue actually "fixed" in mobx-react-lite v4.0.5? or Are there steps that we need to take as developers to be using new mobx in the correct way?

I'll gladly open a new ticket if needed but I am still receiving an Error: Maximum update depth exceeded. bug that only shows up when upgrading from mobx-react-lite v3.4.3 -> v4.0.5 and I'm seriously struggling to debug it, find the root cause, and/or understand how to properly strucutre my app now that mobx is using useSyncExternalStore

[mweststrate]

@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.

[Okoke12022]

flashing error 😭