[Bug] Error: Maximum update depth exceeded #3728
Comments
|
I am not a Next.js expert but the |
|
@kubk That should not matter, we're not calling |
MateuszLeo
changed the title
|
The issue is that observable initialization ( |
|
Quick thought: should we avoid increasing global state version for non-observed atoms? |
|
Imo no, basically for the same reason as noted here #2526 (comment) |
|
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 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 |
|
@jraoult Could you give an example of your implementation? I think having lazy state initialization in The problem is that |
|
@MateuszLeo yes, using 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. |
It's a bug, the plan is to solve it on mobx side, just can't provide an estimate. |
|
We just ran into this and had to revert too. |
|
You can track the progress here #3732 |
|
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. |
|
@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 |
|
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. |
|
We've hit the same issue and wasn't able to find what component actually causes this, rollback to 7.6.0 helps. |
|
On nextjs i found that I was missing mobx's // 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. |
|
@urugator so I finally managed to try this bug fix. I still think there is an issue. Something like that was working before (in 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 Should I create a new issue or is it the expected behaviour? |
|
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<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?
—
Reply to this email directly, view it on GitHub
<#3728 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN4NBB7TGKREJPPULFWIDLXZCU25ANCNFSM6AAAAAA2N5BIB4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
@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 I guess my real question is why an update to an observable local to a component triggers a re-render of the parent component? |
In this specific example, you get
It's not really "local" in terms of some kind of isolation. It's just an observable created on mount and held in |
|
@urugator many thanks for your reply and the explanation of the logic behind that said my question remains:
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 The fix is making the value passed though |
|
I think I want to dig into this one a bit deeper. Some random thoughts:
https://stackblitz.com/edit/stackblitz-starters-jxldkn?file=src%2FApp.tsx |
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);
}
})
);
It has nothing to do with tracking, it's because of how |
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 Whatever works best for you, just keep in mind that
Also notice what |
|
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:
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 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 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 This is a fair assertion, and our performance-sensitive large pages/tables actually do this--however, I assert the React mental model is that (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 |
|
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:
Yep! This is my question/confusion as well. In @urugator 's assertion that "all of mobx is a global store anyway":
Honestly I don't know the internal Mobx implementation details pre- 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-
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 |
|
@stephenh Not sure it is relevant, but I also crafted a version of my reduction but using Valtio instead (that is fundamentally based on |
|
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 Or Note that making the effect async (e.g. something like 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. |
@mweststrate there you go: same thing but with valtio 😉 |
|
at the risk of being lazy: mind adding a store read to |
|
@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. #3728 (comment)). |
|
yeah I'm aware, but there is |
|
Valtio has basically the same problem: 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 |
|
@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 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>;
}; |
@mweststrate brilliant, that might be my way forward then 👏 |
|
@mweststrate (and @urugator / @jraoult ) really appreciate the time & reply!
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 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 Afaiu this local counter, with (Fwiw this is what legend-state's useSelector does, which is like their 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 |
We actually do that if globalVersion isn't available: mobx/packages/mobx-react-lite/src/useObserver.ts Lines 16 to 20 in 91adf79 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.
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.
It would be suprising to me, if you wouldn't need a mechanism to compensate for these, but it's a possibility.
It's valid, it's not idiomatic: My guess is, that if you wouldn't use Mobx, you wouldn't even move the prop to local state. I think you use |
100%. We have a complicated
In this simplified example, you're right, but in the real component we use 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? 🤷
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 Kinda wonder, given ...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"`: So I guess I'll have to stay with "map props -> stores from a Fwiw if you look at legend-state's useSelector, the way they use Could mobx just do what legend-state is doing? No global version number, and no I haven't had a chance to fork For your points, @urugator , about Fwiw/either way, I think we can all agree that while |
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.
If you mean "in canonical
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 +
You're again right--store mutations won't be communicated via props/context, but they will be communicated via reactions to Particularly to the last point, it seems the global version number totally breaks fine-grained reactivity? If every component with 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 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 |
|
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
<#3728 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN4NBD22AJHPZAH3VKUAALX2KJJXANCNFSM6AAAAAA2N5BIB4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…rops not propagationg, fixes #3728
|
We partially reverted the behavior for now in [email protected]. 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. |
|
Awesome! Thank you @mweststrate !
Fwiw/AFAIU the current implementation (where you removed the global version check) still provides tear-free concurrency support... I.e. if we render The 🤔 / debate AFAIU is whether a separate, non-dependent property (potentially in a completely separate observable/completely separate store) like Imo it shouldn't, b/c if ...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. 🤷 Either way, really appreciate your work! Thank you! |
* fix: disable global state version usage to avoid footgun with fresh props not propagationg, fixes #3728 * chore: Added changeset * chore: cleanup global state version
|
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 |
|
@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 |
This comment was marked as off-topic.
This comment was marked as off-topic.
Intended outcome:
Render without any issues
Actual outcome:
Error: Maximum update depth exceeded.How to reproduce the issue:
Repro:
_app.jsx- define the component asobserverindex.jsx- useuseStateto storemobxstore without lazy initialization (useState(new Store()))Maximum update depth exceededAny component defined as
observerhigher in react tree than the definedmobxstore is causing the issue above.Lazy initialization (
useState(() => new Store())), downgrading mobx-react-lite to 3.4.3 fixses issue or removingobserverfixes issue.Versions
[email protected]
The text was updated successfully, but these errors were encountered: