[mobx-react] Maximum update depth exceeded error since mobx-react@8

[jeffijoe]

Intended outcome:

There should be no Maximum update depth exceeded error

Actual outcome:

React throws a Maximum update depth exceeded error.

How to reproduce the issue:

I created a reproduction repository (see src/MobX.jsx) - it includes both mobx and downshift, this is because downshift is the root cause however the way mobx-react behaves after version 8 (with the change to useSyncExternalStore) makes me believe it's worth investigating on the mobx-react side.

You'll be able to see for yourself by downgrading to mobx-react@7 in that repo and running the test - it will show 2 in the counter instead of 53 (at which point the error is thrown which stops the loop).

I know the issue template states to avoid speculation but I've spent way too long investigating what's going on to not not mention it 😅 What seems to be happening is the useSyncExternalStore will "push" another work item on the internal React sync queue (for batching) and because every reaction will make mobx-react's implementation return a new symbol in getSnapshot, if React triggers an effect which subsequently triggers a MobX reaction, it will keep pushing to that queue for each iteration.

mobx/packages/mobx-react-lite/src/useObserver.ts

Lines 49 to 72 in 13a74fe

	subscribe(onStoreChange: () => void) {
	// Do NOT access admRef here!
	observerFinalizationRegistry.unregister(adm)
	adm.onStoreChange = onStoreChange
	if (!adm.reaction) {
	// We've lost our reaction and therefore all subscriptions, occurs when:
	// 1. Timer based finalization registry disposed reaction before component mounted.
	// 2. React "re-mounts" same component without calling render in between (typically <StrictMode>).
	// We have to recreate reaction and schedule re-render to recreate subscriptions,
	// even if state did not change.
	createReaction(adm)
	// `onStoreChange` won't force update if subsequent `getSnapshot` returns same value.
	// So we make sure that is not the case
	adm.stateVersion = Symbol()
	}
	return () => {
	// Do NOT access admRef here!
	adm.onStoreChange = null
	adm.reaction?.dispose()
	adm.reaction = null
	}
	},
	getSnapshot() {

New work pushed to queue

As I mentioned, the root cause is a different library, but what prompted me to look into this was because in my real application, the browser tap freezes when this happens as the error actually loops infinitely there. I couldn't reproduce that here though. Additionally, when using plain React useState (as seen in Basic.jsx), it does not result in the error.

I have also opened an issue with downshift.

Versions

• mobx@6.12.3
• mobx-react@9.1.1
• react@18.2.0
• react-dom@18.2.0

[mweststrate]

Eh... that test description definitely explains issues I've seen around automation with other projects in the past, even not MobX related 😅

curious if #3863 has an effect on this

[jeffijoe]

If you can release it for preview I can give it a shot!

[zhantx]

Hi @mweststrate my PR won't have any effect on this as the PR is for class component only.
for this issue, I spent some time to investigate. I believe this is not a mobx-react-lite issue, thus I suggest this issue should be closed

1. I tried applying a similar approach I did that introduces the pendingStateVersion, so that we prevent calling onStoreChange multiple times before re-render, in theory, one onStoreChange call is enough to let react to schedule the update. However this doesn't fix the issue
2. Upon on further investigation, I can see onSelectedItemChange from downshift is called during commit phase, essentially what happened in this example is
   user clicks -> onSelectedItemChange called during commit phase -> mutate the observable store -> useObserver's reaction runs -> onStoreChange() so react schedule an update in SyncLane -> react renders this component, we now consider the component is up-to-date -> in the commit phase, downshift calls onSelectedItemChange again -> mutate the observable store -> useObserver's reaction runs -> ...... loop

The problem here is downshift calls onSelectedItemChange again at commit phase. mobx-react-lite does everything correctly

cc @jeffijoe

[jeffijoe]

@zhantx for context, when using regular React state (without mobx) in the Downshift reproduction, it only runs twice instead of looping, that's why I opened an issue here as well.

[zhantx]

That's right, let me explain
When a component needs to update (i.e. setState from useState hook is called), react schedule/enqueue an update
then, react starts a render cycle, which consists of render phase (react calls render function) and commit phase (react commit DOM, react calls effects)

for regular React state

1. downshift calls onSelectedItemChange in commit phase
2. this means react's setState is called during commit phase, by default react logic, setState enqueues an update in DefaultLane
3. For DefaultLane update, react flush the work in next render cycle (you can think next render is scheduled by setTimeout(0))

when using mobx, because mobx is an external store, naturally, it uses useSyncExternalStore hook

1. downshift calls onSelectedItemChange in commit phase
2. mobx store is mutated within onSelectedItemChange, mobx-react notify react to enqueue a SyncLane update
3. For SyncLane update, because react is at commit phase, react will work on SyncLane update immediately, it essentially go back to render phase in current cycle, when all render works are done, react will enter commit phase again
4. for some reason (I couldn't dig too deep into downshift), downshift calls onSelectedItemChange when react enters commit phase again, thus causes the loop

It seems to me that mobx-react is doing what we expect, that if any store mutates after render phase, we should let react to re-render the component. and because mobx is an external store, any updates should be in SyncLane
This is more background on useSyncExternalStore and why mobx-react should use it