Skip to content

Commit 0a8854a

Browse files
committed
Fix #3492: throw warning when use class component and suspense
1 parent f0e066f commit 0a8854a

8 files changed

+555
-4
lines changed

.changeset/chilly-nails-own.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"mobx-react": patch
3+
---
4+
5+
Fix #3492: throw warning when use class component and suspense
Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,216 @@
1+
import { act, cleanup, render } from "@testing-library/react"
2+
import * as mobx from "mobx"
3+
import * as React from "react"
4+
5+
import { makeClassComponentObserver } from "../src/observerClass"
6+
import {
7+
forceCleanupTimerToRunNowForTests,
8+
resetCleanupScheduleForTests
9+
} from "../src/utils/reactionCleanupTracking"
10+
import {
11+
CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS,
12+
CLEANUP_TIMER_LOOP_MILLIS
13+
} from "../src/utils/reactionCleanupTrackingCommon"
14+
15+
afterEach(cleanup)
16+
17+
test("uncommitted components should not leak observations", async () => {
18+
resetCleanupScheduleForTests()
19+
20+
// Unfortunately, Jest fake timers don't mock out Date.now, so we fake
21+
// that out in parallel to Jest useFakeTimers
22+
let fakeNow = Date.now()
23+
jest.useFakeTimers()
24+
jest.spyOn(Date, "now").mockImplementation(() => fakeNow)
25+
26+
const store = mobx.observable({ count1: 0, count2: 0 })
27+
28+
// Track whether counts are observed
29+
let count1IsObserved = false
30+
let count2IsObserved = false
31+
mobx.onBecomeObserved(store, "count1", () => (count1IsObserved = true))
32+
mobx.onBecomeUnobserved(store, "count1", () => (count1IsObserved = false))
33+
mobx.onBecomeObserved(store, "count2", () => (count2IsObserved = true))
34+
mobx.onBecomeUnobserved(store, "count2", () => (count2IsObserved = false))
35+
36+
class TestComponent1_ extends React.PureComponent {
37+
render() {
38+
return <div>{store.count1}</div>
39+
}
40+
}
41+
42+
const TestComponent1 = makeClassComponentObserver(TestComponent1_)
43+
44+
class TestComponent2_ extends React.PureComponent {
45+
render() {
46+
return <div>{store.count2}</div>
47+
}
48+
}
49+
50+
const TestComponent2 = makeClassComponentObserver(TestComponent2_)
51+
52+
// Render, then remove only #2
53+
const rendering = render(
54+
<React.StrictMode>
55+
<TestComponent1 />
56+
<TestComponent2 />
57+
</React.StrictMode>
58+
)
59+
rendering.rerender(
60+
<React.StrictMode>
61+
<TestComponent1 />
62+
</React.StrictMode>
63+
)
64+
65+
// Allow any reaction-disposal cleanup timers to run
66+
const skip = Math.max(CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, CLEANUP_TIMER_LOOP_MILLIS)
67+
fakeNow += skip
68+
jest.advanceTimersByTime(skip)
69+
70+
// count1 should still be being observed by Component1,
71+
// but count2 should have had its reaction cleaned up.
72+
expect(count1IsObserved).toBeTruthy()
73+
expect(count2IsObserved).toBeFalsy()
74+
})
75+
76+
test("cleanup timer should not clean up recently-pended reactions", () => {
77+
// If we're not careful with timings, it's possible to get the
78+
// following scenario:
79+
// 1. Component instance A is being created; it renders, we put its reaction R1 into the cleanup list
80+
// 2. Strict/Concurrent mode causes that render to be thrown away
81+
// 3. Component instance A is being created; it renders, we put its reaction R2 into the cleanup list
82+
// 4. The MobX reaction timer from 5 seconds ago kicks in and cleans up all reactions from uncommitted
83+
// components, including R1 and R2
84+
// 5. The commit phase runs for component A, but reaction R2 has already been disposed. Game over.
85+
86+
// This unit test attempts to replicate that scenario:
87+
resetCleanupScheduleForTests()
88+
89+
// Unfortunately, Jest fake timers don't mock out Date.now, so we fake
90+
// that out in parallel to Jest useFakeTimers
91+
const fakeNow = Date.now()
92+
jest.useFakeTimers()
93+
jest.spyOn(Date, "now").mockImplementation(() => fakeNow)
94+
95+
const store = mobx.observable({ count: 0 })
96+
97+
// Track whether the count is observed
98+
let countIsObserved = false
99+
mobx.onBecomeObserved(store, "count", () => (countIsObserved = true))
100+
mobx.onBecomeUnobserved(store, "count", () => (countIsObserved = false))
101+
102+
class TestComponent1_ extends React.PureComponent {
103+
render() {
104+
return <div>{store.count}</div>
105+
}
106+
}
107+
108+
const TestComponent1 = makeClassComponentObserver(TestComponent1_)
109+
110+
const rendering = render(
111+
// We use StrictMode here, but it would be helpful to switch this to use real
112+
// concurrent mode: we don't have a true async render right now so this test
113+
// isn't as thorough as it could be.
114+
<React.StrictMode>
115+
<TestComponent1 />
116+
</React.StrictMode>
117+
)
118+
119+
// We need to trigger our cleanup timer to run. We can't do this simply
120+
// by running all jest's faked timers as that would allow the scheduled
121+
// `useEffect` calls to run, and we want to simulate our cleanup timer
122+
// getting in between those stages.
123+
124+
// We force our cleanup loop to run even though enough time hasn't _really_
125+
// elapsed. In theory, it won't do anything because not enough time has
126+
// elapsed since the reactions were queued, and so they won't be disposed.
127+
forceCleanupTimerToRunNowForTests()
128+
129+
// Advance time enough to allow any timer-queued effects to run
130+
jest.advanceTimersByTime(500)
131+
132+
// Now allow the useEffect calls to run to completion.
133+
act(() => {
134+
// no-op, but triggers effect flushing
135+
})
136+
137+
// count should still be observed
138+
expect(countIsObserved).toBeTruthy()
139+
})
140+
141+
// TODO: MWE: disabled during React 18 migration, not sure how to express it icmw with testing-lib,
142+
// and using new React renderRoot will fail icmw JSDOM
143+
test.skip("component should recreate reaction if necessary", () => {
144+
// There _may_ be very strange cases where the reaction gets tidied up
145+
// but is actually still needed. This _really_ shouldn't happen.
146+
// e.g. if we're using Suspense and the component starts to render,
147+
// but then gets paused for 60 seconds, and then comes back to life.
148+
// With the implementation of React at the time of writing this, React
149+
// will actually fully re-render that component (discarding previous
150+
// hook slots) before going ahead with a commit, but it's unwise
151+
// to depend on such an implementation detail. So we must cope with
152+
// the component having had its reaction tidied and still going on to
153+
// be committed. In that case we recreate the reaction and force
154+
// an update.
155+
156+
// This unit test attempts to replicate that scenario:
157+
158+
resetCleanupScheduleForTests()
159+
160+
// Unfortunately, Jest fake timers don't mock out Date.now, so we fake
161+
// that out in parallel to Jest useFakeTimers
162+
let fakeNow = Date.now()
163+
jest.useFakeTimers()
164+
jest.spyOn(Date, "now").mockImplementation(() => fakeNow)
165+
166+
const store = mobx.observable({ count: 0 })
167+
168+
// Track whether the count is observed
169+
let countIsObserved = false
170+
mobx.onBecomeObserved(store, "count", () => (countIsObserved = true))
171+
mobx.onBecomeUnobserved(store, "count", () => (countIsObserved = false))
172+
173+
class TestComponent1_ extends React.PureComponent {
174+
render() {
175+
return <div>{store.count}</div>
176+
}
177+
}
178+
179+
const TestComponent1 = makeClassComponentObserver(TestComponent1_)
180+
181+
const rendering = render(
182+
<React.StrictMode>
183+
<TestComponent1 />
184+
</React.StrictMode>
185+
)
186+
187+
// We need to trigger our cleanup timer to run. We don't want
188+
// to allow Jest's effects to run, however: we want to simulate the
189+
// case where the component is rendered, then the reaction gets cleaned up,
190+
// and _then_ the component commits.
191+
192+
// Force everything to be disposed.
193+
const skip = Math.max(CLEANUP_LEAKED_REACTIONS_AFTER_MILLIS, CLEANUP_TIMER_LOOP_MILLIS)
194+
fakeNow += skip
195+
forceCleanupTimerToRunNowForTests()
196+
197+
// The reaction should have been cleaned up.
198+
expect(countIsObserved).toBeFalsy()
199+
200+
// Whilst nobody's looking, change the observable value
201+
store.count = 42
202+
203+
// Now allow the useEffect calls to run to completion,
204+
// re-awakening the component.
205+
jest.advanceTimersByTime(500)
206+
act(() => {
207+
// no-op, but triggers effect flushing
208+
})
209+
210+
// count should be observed once more.
211+
expect(countIsObserved).toBeTruthy()
212+
// and the component should have rendered enough to
213+
// show the latest value, which was set whilst it
214+
// wasn't even looking.
215+
expect(rendering.baseElement.textContent).toContain("42")
216+
})

packages/mobx-react/src/observerClass.ts

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,11 @@ import {
1010
import { isUsingStaticRendering } from "mobx-react-lite"
1111

1212
import { newSymbol, shallowEqual, setHiddenProp, patch } from "./utils/utils"
13+
import {
14+
addReactionToTrack,
15+
reactionTrack,
16+
recordReactionAsCommitted
17+
} from "./utils/reactionCleanupTracking"
1318

1419
const mobxAdminProperty = $mobx || "$mobx" // BC
1520
const mobxObserverProperty = newSymbol("isMobXReactObserver")
@@ -73,9 +78,26 @@ export function makeClassComponentObserver(
7378
}
7479
patch(target, "componentDidMount", function () {
7580
this[mobxIsUnmounted] = false
76-
if (!this.render[mobxAdminProperty]) {
77-
// Reaction is re-created automatically during render, but a component can re-mount and skip render #3395.
78-
// To re-create the reaction and re-subscribe to relevant observables we have to force an update.
81+
recordReactionAsCommitted(this)
82+
if (this[reactionTrack]) {
83+
this[reactionTrack].mounted = true
84+
if (!this.render[mobxAdminProperty] || this[reactionTrack].changedBeforeMount) {
85+
// Reaction is re-created automatically during render, but a component can re-mount and skip render #3395.
86+
// To re-create the reaction and re-subscribe to relevant observables we have to force an update.
87+
Component.prototype.forceUpdate.call(this)
88+
this[reactionTrack].changedBeforeMount = false
89+
}
90+
} else {
91+
const initialName = getDisplayName(this)
92+
this[reactionTrack] = {
93+
reaction: new Reaction(`${initialName}.render()`, () => {
94+
// We've definitely already been mounted at this point
95+
Component.prototype.forceUpdate.call(this)
96+
}),
97+
mounted: true,
98+
changedBeforeMount: false,
99+
cleanAt: Infinity
100+
}
79101
Component.prototype.forceUpdate.call(this)
80102
}
81103
})
@@ -143,7 +165,11 @@ function createReactiveRender(originalRender: any) {
143165
try {
144166
setHiddenProp(this, isForcingUpdateKey, true)
145167
if (!this[skipRenderKey]) {
146-
Component.prototype.forceUpdate.call(this)
168+
if (this[reactionTrack].mounted) {
169+
Component.prototype.forceUpdate.call(this)
170+
} else {
171+
this[reactionTrack].changedBeforeMount = true
172+
}
147173
}
148174
hasError = false
149175
} finally {
@@ -165,6 +191,11 @@ function createReactiveRender(originalRender: any) {
165191
isRenderingPending = false
166192
// Create reaction lazily to support re-mounting #3395
167193
const reaction = (reactiveRender[mobxAdminProperty] ??= createReaction())
194+
195+
if (!this[reactionTrack]) {
196+
addReactionToTrack(this, reaction, {})
197+
}
198+
168199
let exception: unknown = undefined
169200
let rendering = undefined
170201
reaction.track(() => {
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
declare class FinalizationRegistryType<T> {
2+
constructor(cleanup: (cleanupToken: T) => void)
3+
register(object: object, cleanupToken: T, unregisterToken?: object): void
4+
unregister(unregisterToken: object): void
5+
}
6+
7+
declare const FinalizationRegistry: typeof FinalizationRegistryType | undefined
8+
9+
const FinalizationRegistryLocal =
10+
typeof FinalizationRegistry === "undefined" ? undefined : FinalizationRegistry
11+
12+
export { FinalizationRegistryLocal as FinalizationRegistry }
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
import { FinalizationRegistry as FinalizationRegistryMaybeUndefined } from "./FinalizationRegistryWrapper"
2+
import { Reaction } from "mobx"
3+
import {
4+
ReactionCleanupTracking,
5+
IReactionTracking,
6+
createTrackingData,
7+
Comp,
8+
reactionTrack
9+
} from "./reactionCleanupTrackingCommon"
10+
11+
/**
12+
* FinalizationRegistry-based uncommitted reaction cleanup
13+
*/
14+
export function createReactionCleanupTrackingUsingFinalizationRegister(
15+
FinalizationRegistry: NonNullable<typeof FinalizationRegistryMaybeUndefined>
16+
): ReactionCleanupTracking {
17+
const cleanupTokenToReactionTrackingMap = new Map<number, IReactionTracking>()
18+
let globalCleanupTokensCounter = 1
19+
20+
const registry = new FinalizationRegistry(function cleanupFunction(token: number) {
21+
const trackedReaction = cleanupTokenToReactionTrackingMap.get(token)
22+
if (trackedReaction) {
23+
trackedReaction.reaction.dispose()
24+
cleanupTokenToReactionTrackingMap.delete(token)
25+
}
26+
})
27+
28+
return {
29+
/**
30+
* TS type derivation cannot recognize symbol here. Manually set the type
31+
*/
32+
addReactionToTrack(
33+
reactionTrackingRef: Comp,
34+
reaction: Reaction,
35+
objectRetainedByReact: object
36+
) {
37+
const token = globalCleanupTokensCounter++
38+
39+
registry.register(objectRetainedByReact, token, reactionTrackingRef)
40+
reactionTrackingRef[reactionTrack] = createTrackingData(reaction)
41+
;(
42+
reactionTrackingRef[reactionTrack] as IReactionTracking
43+
).finalizationRegistryCleanupToken = token
44+
cleanupTokenToReactionTrackingMap.set(
45+
token,
46+
reactionTrackingRef[reactionTrack] as IReactionTracking
47+
)
48+
49+
return reactionTrackingRef[reactionTrack] as IReactionTracking
50+
},
51+
recordReactionAsCommitted(reactionRef: Comp) {
52+
registry.unregister(reactionRef)
53+
54+
if (
55+
reactionRef[reactionTrack] &&
56+
(reactionRef[reactionTrack] as IReactionTracking).finalizationRegistryCleanupToken
57+
) {
58+
cleanupTokenToReactionTrackingMap.delete(
59+
(reactionRef[reactionTrack] as IReactionTracking)
60+
.finalizationRegistryCleanupToken as number
61+
)
62+
}
63+
},
64+
forceCleanupTimerToRunNowForTests() {
65+
// When FinalizationRegistry in use, this this is no-op
66+
},
67+
resetCleanupScheduleForTests() {
68+
// When FinalizationRegistry in use, this this is no-op
69+
}
70+
}
71+
}

0 commit comments

Comments
 (0)