Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #3728: observables initialization should not update state version #3732

Merged
merged 10 commits into from
Aug 26, 2023
Merged

Fix #3728: observables initialization should not update state version #3732

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4e18509
wip
urugator Jul 23, 2023
a0431e8
wip
urugator Jul 23, 2023
e9cc9c3
batchId
urugator Aug 12, 2023
750eedb
Merge remote-tracking branch 'upstream/main' into fix-3728
urugator Aug 12, 2023
20a7106
fix isolateGlobalState
urugator Aug 12, 2023
1323692
tinker with timers
urugator Aug 12, 2023
192e7c0
initObservable: flag order
urugator Aug 12, 2023
8a5595f
fix test description
urugator Aug 26, 2023
77e617c
fix changeset
urugator Aug 26, 2023
6e81e91
GC time tinkering
urugator Aug 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/spotty-emus-hear.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"mobx": patch
---

- fix: #3728: Observable initialization updates state version.
- fix: Observable set initialization violates `enforceActions: "always"`.
- fix: Changing keys of observable object does not respect `enforceActions`.
96 changes: 96 additions & 0 deletions packages/mobx/__tests__/v5/base/observables.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2387,3 +2387,99 @@ test("state version updates correctly", () => {
expect(o.x).toBe(5)
expect(prevStateVersion).not.toBe(getGlobalState().stateVersion)
})

test('Observables initialization does not violate `enforceActions: "always"`', () => {
const consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(() => {})

const check = cb => {
cb()
expect(consoleWarnSpy).not.toBeCalled()
}

class MakeObservable {
x = 0
constructor() {
mobx.makeObservable(this, { x: true })
}
}
class MakeAutoObservable {
x = 0
constructor() {
mobx.makeAutoObservable(this)
}
}

try {
mobx.configure({ enforceActions: "always" })
check(() => mobx.observable(0))
check(() => new MakeObservable())
check(() => mobx.makeObservable({ x: 0 }, { x: true }))
check(() => new MakeAutoObservable())
check(() => mobx.makeAutoObservable({ x: 0 }))
check(() => mobx.observable(new Set([0])))
check(() => mobx.observable(new Map([[0, 0]])))
check(() => mobx.observable({ x: 0 }, { proxy: false }))
check(() => mobx.observable({ x: 0 }, { proxy: true }))
check(() => mobx.observable([0], { proxy: false }))
check(() => mobx.observable([0], { proxy: true }))
check(() => mobx.computed(() => 0))
} finally {
consoleWarnSpy.mockRestore()
mobx._resetGlobalState()
}
})

test("enforceAction is respected when changing keys of observable object", () => {
const consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(() => {})
try {
mobx.configure({ enforceActions: "always" })
const o = mobx.observable({ x: 0 })

o.y = 0
expect(consoleWarnSpy).toBeCalled()

consoleWarnSpy.mockClear()

delete o.x
expect(consoleWarnSpy).toBeCalled()
} finally {
consoleWarnSpy.mockRestore()
mobx._resetGlobalState()
}
})

test("state version does not update on observable creation", () => {
const globalState = getGlobalState()

const check = cb => {
const prevStateVersion = globalState.stateVersion
cb()
expect(prevStateVersion).toBe(globalState.stateVersion)
}

class MakeObservable {
x = 0
constructor() {
mobx.makeObservable(this, { x: true })
}
}
class MakeAutoObservable {
x = 0
constructor() {
mobx.makeAutoObservable(this)
}
}

check(() => mobx.observable(0))
check(() => new MakeObservable())
check(() => mobx.makeObservable({ x: 0 }, { x: true }))
check(() => new MakeAutoObservable())
check(() => mobx.makeAutoObservable({ x: 0 }))
check(() => mobx.observable(new Set([0])))
check(() => mobx.observable(new Map([[0, 0]])))
check(() => mobx.observable({ x: 0 }, { proxy: false }))
check(() => mobx.observable({ x: 0 }, { proxy: true }))
check(() => mobx.observable([0], { proxy: false }))
check(() => mobx.observable([0], { proxy: true }))
check(() => mobx.computed(() => 0))
})
9 changes: 8 additions & 1 deletion packages/mobx/src/api/extendobservable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ import {
die,
getOwnPropertyDescriptors,
$mobx,
ownKeys
ownKeys,
globalState,
allowStateChangesStart,
allowStateChangesEnd
} from "../internal"

export function extendObservable<A extends Object, B extends Object>(
Expand Down Expand Up @@ -41,6 +44,8 @@ export function extendObservable<A extends Object, B extends Object>(
const descriptors = getOwnPropertyDescriptors(properties)

const adm: ObservableObjectAdministration = asObservableObject(target, options)[$mobx]
const allowStateChanges = allowStateChangesStart(true)
globalState.suppressReportChanged = true
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan is to change these into suppressReportChangedStart/End and probably move it to initObservableStart/End (together with unstrackedStart/End and allowStateChangesStart/End as noted in comments).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, some dedicated utility sounds great here to avoid copy / paste mistakes.

startBatch()
try {
ownKeys(descriptors).forEach(key => {
Expand All @@ -52,6 +57,8 @@ export function extendObservable<A extends Object, B extends Object>(
)
})
} finally {
globalState.suppressReportChanged = false
allowStateChangesEnd(allowStateChanges)
endBatch()
}
return target as any
Expand Down
17 changes: 14 additions & 3 deletions packages/mobx/src/api/makeObservable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ import {
ownKeys,
extendObservable,
addHiddenProp,
storedAnnotationsSymbol
storedAnnotationsSymbol,
globalState,
allowStateChangesStart,
allowStateChangesEnd
} from "../internal"

// Hack based on https://github.com/Microsoft/TypeScript/issues/14829#issuecomment-322267089
Expand All @@ -31,7 +34,9 @@ export function makeObservable<T extends object, AdditionalKeys extends Property
options?: MakeObservableOptions
): T {
const adm: ObservableObjectAdministration = asObservableObject(target, options)[$mobx]
startBatch()
startBatch() // TODO useless?
const allowStateChanges = allowStateChangesStart(true)
globalState.suppressReportChanged = true
try {
if (__DEV__ && annotations && target[storedAnnotationsSymbol]) {
die(
Expand All @@ -44,6 +49,8 @@ export function makeObservable<T extends object, AdditionalKeys extends Property
// Annotate
ownKeys(annotations).forEach(key => adm.make_(key, annotations![key]))
} finally {
globalState.suppressReportChanged = false
allowStateChangesEnd(allowStateChanges)
endBatch()
}
return target
Expand Down Expand Up @@ -84,7 +91,9 @@ export function makeAutoObservable<T extends object, AdditionalKeys extends Prop
addHiddenProp(proto, keysSymbol, keys)
}

startBatch()
const allowStateChanges = allowStateChangesStart(true)
globalState.suppressReportChanged = true
startBatch() // TODO useless?
try {
target[keysSymbol].forEach(key =>
adm.make_(
Expand All @@ -94,6 +103,8 @@ export function makeAutoObservable<T extends object, AdditionalKeys extends Prop
)
)
} finally {
globalState.suppressReportChanged = false
allowStateChangesEnd(allowStateChanges)
endBatch()
}
return target
Expand Down
3 changes: 3 additions & 0 deletions packages/mobx/src/core/atom.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ export class Atom implements IAtom {
* Invoke this method _after_ this method has changed to signal mobx that all its observers should invalidate.
*/
public reportChanged() {
if (globalState.suppressReportChanged) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a potentially dangerous check, that silently might break reactivity. E.g. do the following cases behave correctly?

const x = observable({ a: 1 })
reaction(x => x.a, a => console.log("a", a))
extendObservable(x, { a: 2, b: 1 })
const x = observable({ a: 1 }, { proxy: true })
// react to new keys
reaction(x => Object.keys(x.a), a => console.log("a", a))

x.b = 1

I'm wondering if an alternative approach would be: only increase stateVersion if there are subscribers to the current atom?

Copy link
Collaborator Author

@urugator urugator Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a potentially dangerous check, that silently might break reactivity

Good point. Imo the play here is to make sure it's suppressed only on creation and not on mutation like in this case.
Key changes should invoke reactions and therefore update state version, these two go hand in hand, a reaction must always see a new state version. Therefore you shouldn't extend observable during render.
Just for clarification: extendObservable does notify about key changes.

only increase stateVersion if there are subscribers to the current atom

Purely from semantic perspective, two different states should aways have different versions, doesn't matter if someone was observing some part of the state or not. That's quite easy to reason about.
Practically speaking, I don't know. I have a bad feeling about it, but I have a hard time to come up with a convincing example. My worry is, that you change some observable x, that nobody is observing, so version stays the same. Then something unrelated to mobx - props/state or other external sync store - changes in such a way, that the next render would read from x. Now if react would check whether mobx external state changed, it doesn't know about the change of x, even though it's about to use it together with rest of the world (other observables, state, props, other external state) to render. I am not sure if that alone is a problem. The goal here is to avoid inconsistencies and to have an inconsistency, the x would probably have to be somehow correlated with the rest of the world (props/state/other store). But then, if they're correlated, they had to change at the same moment and therefore something, somewhere had to already signal a new version? It's hard for me to contemplate about this, especially when I don't know what react exactly does under the hood and what it's assumptions are. If version always changes, I don't have to think about this...

Copy link
Member

@mweststrate mweststrate Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purely from semantic perspective, two different states should aways have different versions, doesn't matter if someone was observing some part of the state or not. That's quite easy to reason about

If version always changes, I don't have to think about this...

Agreed, I think these principles weighs stronger than my concerns. Let's continue this direction.

Copy link
Collaborator Author

@urugator urugator Aug 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a fundamental problem with subclasses - new Subclass() always counts as a state update, because the subclass's constructor calls makeObservable on an already observable object (this).
Strictly speaking it's correct behavior, because constructors can insert any arbitrary logic between individual makeObservable calls and there is no guarantee new Subclass is called from transaction, so a reaction invoked in between these makeObservable calls should see new state version.

What we would need is to treat all modifications of the same atom inside the same batch it was created as part of initialization. To do that we would need to introduce globalState.batchId, assign this batchId to every atom when created and suppress reportChanged when atom.batchId === globalState.batchId. Now you don't have to manually demark initialization sections - they span from atom creation until the end of the current batch. If you call new Subclass() from action as a good citizen, the state version stays same, no matter how many times you called makeObservable. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that sounds smart! It might serve in the future as well to better handle any weird edge cases around reaction creations in actions, recursive updates etc etc, although I believe we handle those all consistently now.

return
}
startBatch()
propagateChanged(this)
// We could update state version only at the end of batch,
Expand Down
5 changes: 5 additions & 0 deletions packages/mobx/src/core/globalstate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,11 @@ export class MobXGlobals {
* Changes with each state update, used by useSyncExternalStore
*/
stateVersion = Number.MIN_SAFE_INTEGER

/**
* TODO
*/
suppressReportChanged = false
}

let canMergeGlobalState = true
Expand Down
16 changes: 11 additions & 5 deletions packages/mobx/src/types/legacyobservablearray.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import {
IEnhancer,
isObservableArray,
IObservableArray,
defineProperty
defineProperty,
globalState
} from "../internal"

// Bug in safari 9.* (or iOS 9 safari mobile). See #364
Expand Down Expand Up @@ -67,10 +68,15 @@ class LegacyObservableArray<T> extends StubArray {
addHiddenFinalProp(this, $mobx, adm)

if (initialValues && initialValues.length) {
const prev = allowStateChangesStart(true)
// @ts-ignore
this.spliceWithArray(0, 0, initialValues)
allowStateChangesEnd(prev)
const allowStateChanges = allowStateChangesStart(true)
globalState.suppressReportChanged = true
try {
// @ts-ignore
this.spliceWithArray(0, 0, initialValues)
} finally {
globalState.suppressReportChanged = false
allowStateChangesEnd(allowStateChanges)
}
}

if (safariPrototypeSetterInheritanceBug) {
Expand Down
11 changes: 8 additions & 3 deletions packages/mobx/src/types/observablearray.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,9 +411,14 @@ export function createObservableArray<T>(
const proxy = new Proxy(adm.values_, arrayTraps) as any
adm.proxy_ = proxy
if (initialValues && initialValues.length) {
const prev = allowStateChangesStart(true)
adm.spliceWithArray_(0, 0, initialValues)
allowStateChangesEnd(prev)
const allowStateChanges = allowStateChangesStart(true)
globalState.suppressReportChanged = true
try {
adm.spliceWithArray_(0, 0, initialValues)
} finally {
globalState.suppressReportChanged = false
allowStateChangesEnd(allowStateChanges)
}
}
return proxy
}
Expand Down
19 changes: 14 additions & 5 deletions packages/mobx/src/types/observablemap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ import {
UPDATE,
IAtom,
PureSpyEvent,
allowStateChanges
allowStateChangesStart,
allowStateChangesEnd
} from "../internal"

export interface IKeyValueMap<V = any> {
Expand Down Expand Up @@ -90,7 +91,8 @@ export type IObservableMapInitialValues<K = any, V = any> =
// just extend Map? See also https://gist.github.com/nestharus/13b4d74f2ef4a2f4357dbd3fc23c1e54
// But: https://github.com/mobxjs/mobx/issues/1556
export class ObservableMap<K = any, V = any>
implements Map<K, V>, IInterceptable<IMapWillChange<K, V>>, IListenable {
implements Map<K, V>, IInterceptable<IMapWillChange<K, V>>, IListenable
{
[$mobx] = ObservableMapMarker
data_: Map<K, ObservableValue<V>>
hasMap_: Map<K, ObservableValue<boolean>> // hasMap, not hashMap >-).
Expand All @@ -110,9 +112,16 @@ export class ObservableMap<K = any, V = any>
this.keysAtom_ = createAtom(__DEV__ ? `${this.name_}.keys()` : "ObservableMap.keys()")
this.data_ = new Map()
this.hasMap_ = new Map()
allowStateChanges(true, () => {
this.merge(initialData)
})
if (initialData) {
const allowStateChanges = allowStateChangesStart(true)
globalState.suppressReportChanged = true
try {
this.merge(initialData)
} finally {
globalState.suppressReportChanged = false
allowStateChangesEnd(allowStateChanges)
}
}
}

private has_(key: K): boolean {
Expand Down
7 changes: 6 additions & 1 deletion packages/mobx/src/types/observableobject.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ import {
getAdministration,
getDebugName,
objectPrototype,
MakeResult
MakeResult,
checkIfStateModificationsAreAllowed
} from "../internal"

const descriptorCache = Object.create(null)
Expand Down Expand Up @@ -314,6 +315,7 @@ export class ObservableObjectAdministration
descriptor: PropertyDescriptor,
proxyTrap: boolean = false
): boolean | null {
checkIfStateModificationsAreAllowed(this.keysAtom_)
try {
startBatch()

Expand Down Expand Up @@ -368,6 +370,7 @@ export class ObservableObjectAdministration
enhancer: IEnhancer<any>,
proxyTrap: boolean = false
): boolean | null {
checkIfStateModificationsAreAllowed(this.keysAtom_)
try {
startBatch()

Expand Down Expand Up @@ -432,6 +435,7 @@ export class ObservableObjectAdministration
options: IComputedValueOptions<any>,
proxyTrap: boolean = false
): boolean | null {
checkIfStateModificationsAreAllowed(this.keysAtom_)
try {
startBatch()

Expand Down Expand Up @@ -490,6 +494,7 @@ export class ObservableObjectAdministration
* @returns {boolean|null} true on success, false on failure (proxyTrap + non-configurable), null when cancelled by interceptor
*/
delete_(key: PropertyKey, proxyTrap: boolean = false): boolean | null {
checkIfStateModificationsAreAllowed(this.keysAtom_)
// No such prop
if (!hasProp(this.target_, key)) {
return true
Expand Down
14 changes: 12 additions & 2 deletions packages/mobx/src/types/observableset.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ import {
DELETE,
ADD,
die,
isFunction
isFunction,
allowStateChangesStart,
globalState,
allowStateChangesEnd
} from "../internal"

const ObservableSetMarker = {}
Expand Down Expand Up @@ -82,7 +85,14 @@ export class ObservableSet<T = any> implements Set<T>, IInterceptable<ISetWillCh
this.atom_ = createAtom(this.name_)
this.enhancer_ = (newV, oldV) => enhancer(newV, oldV, name_)
if (initialData) {
this.replace(initialData)
const allowStateChanges = allowStateChangesStart(true)
globalState.suppressReportChanged = true
try {
this.replace(initialData)
} finally {
globalState.suppressReportChanged = false
allowStateChangesEnd(allowStateChanges)
}
}
}

Expand Down
Loading