From 4e18509f74a40653d6c8671f24c715a23ed0dd43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 23 Jul 2023 09:53:21 +0200 Subject: [PATCH 1/9] wip --- packages/mobx/__tests__/v5/base/observables.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/mobx/__tests__/v5/base/observables.js b/packages/mobx/__tests__/v5/base/observables.js index 08c6b4d96..e970a24a0 100644 --- a/packages/mobx/__tests__/v5/base/observables.js +++ b/packages/mobx/__tests__/v5/base/observables.js @@ -2387,3 +2387,15 @@ test("state version updates correctly", () => { expect(o.x).toBe(5) expect(prevStateVersion).not.toBe(getGlobalState().stateVersion) }) + +test.only("state version is not updated on observable creation", () => { + const globalState = getGlobalState() + let prevStateVersion = globalState.stateVersion + //mobx.observable({ x: 0 }) + // mobx.observable.box(0) + //mobx.observable.map([["key", "val"]]) + mobx.configure({ enforceActions: "always" }) + mobx.makeAutoObservable({ x: "x" }) + + expect(prevStateVersion).toBe(globalState.stateVersion) +}) From a0431e88a4efb583410e65a2693dcf32d8eae144 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sun, 23 Jul 2023 13:55:28 +0200 Subject: [PATCH 2/9] wip --- .changeset/spotty-emus-hear.md | 7 ++ .../mobx/__tests__/v5/base/observables.js | 102 ++++++++++++++++-- packages/mobx/src/api/extendobservable.ts | 9 +- packages/mobx/src/api/makeObservable.ts | 17 ++- packages/mobx/src/core/atom.ts | 3 + packages/mobx/src/core/globalstate.ts | 5 + .../mobx/src/types/legacyobservablearray.ts | 16 ++- packages/mobx/src/types/observablearray.ts | 11 +- packages/mobx/src/types/observablemap.ts | 19 +++- packages/mobx/src/types/observableobject.ts | 7 +- packages/mobx/src/types/observableset.ts | 14 ++- 11 files changed, 181 insertions(+), 29 deletions(-) create mode 100644 .changeset/spotty-emus-hear.md diff --git a/.changeset/spotty-emus-hear.md b/.changeset/spotty-emus-hear.md new file mode 100644 index 000000000..12252cb44 --- /dev/null +++ b/.changeset/spotty-emus-hear.md @@ -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`. diff --git a/packages/mobx/__tests__/v5/base/observables.js b/packages/mobx/__tests__/v5/base/observables.js index e970a24a0..b670131c7 100644 --- a/packages/mobx/__tests__/v5/base/observables.js +++ b/packages/mobx/__tests__/v5/base/observables.js @@ -2388,14 +2388,98 @@ test("state version updates correctly", () => { expect(prevStateVersion).not.toBe(getGlobalState().stateVersion) }) -test.only("state version is not updated on observable creation", () => { +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() - let prevStateVersion = globalState.stateVersion - //mobx.observable({ x: 0 }) - // mobx.observable.box(0) - //mobx.observable.map([["key", "val"]]) - mobx.configure({ enforceActions: "always" }) - mobx.makeAutoObservable({ x: "x" }) - - expect(prevStateVersion).toBe(globalState.stateVersion) + + 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)) }) diff --git a/packages/mobx/src/api/extendobservable.ts b/packages/mobx/src/api/extendobservable.ts index 7b223152a..b9a2058ab 100644 --- a/packages/mobx/src/api/extendobservable.ts +++ b/packages/mobx/src/api/extendobservable.ts @@ -11,7 +11,10 @@ import { die, getOwnPropertyDescriptors, $mobx, - ownKeys + ownKeys, + globalState, + allowStateChangesStart, + allowStateChangesEnd } from "../internal" export function extendObservable( @@ -41,6 +44,8 @@ export function extendObservable( const descriptors = getOwnPropertyDescriptors(properties) const adm: ObservableObjectAdministration = asObservableObject(target, options)[$mobx] + const allowStateChanges = allowStateChangesStart(true) + globalState.suppressReportChanged = true startBatch() try { ownKeys(descriptors).forEach(key => { @@ -52,6 +57,8 @@ export function extendObservable( ) }) } finally { + globalState.suppressReportChanged = false + allowStateChangesEnd(allowStateChanges) endBatch() } return target as any diff --git a/packages/mobx/src/api/makeObservable.ts b/packages/mobx/src/api/makeObservable.ts index ffb02e85e..51a8642e1 100644 --- a/packages/mobx/src/api/makeObservable.ts +++ b/packages/mobx/src/api/makeObservable.ts @@ -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 @@ -31,7 +34,9 @@ export function makeObservable adm.make_(key, annotations![key])) } finally { + globalState.suppressReportChanged = false + allowStateChangesEnd(allowStateChanges) endBatch() } return target @@ -84,7 +91,9 @@ export function makeAutoObservable adm.make_( @@ -94,6 +103,8 @@ export function makeAutoObservable 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) { diff --git a/packages/mobx/src/types/observablearray.ts b/packages/mobx/src/types/observablearray.ts index d5e46a03a..8136a7341 100644 --- a/packages/mobx/src/types/observablearray.ts +++ b/packages/mobx/src/types/observablearray.ts @@ -411,9 +411,14 @@ export function createObservableArray( 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 } diff --git a/packages/mobx/src/types/observablemap.ts b/packages/mobx/src/types/observablemap.ts index e005df972..62db97457 100644 --- a/packages/mobx/src/types/observablemap.ts +++ b/packages/mobx/src/types/observablemap.ts @@ -35,7 +35,8 @@ import { UPDATE, IAtom, PureSpyEvent, - allowStateChanges + allowStateChangesStart, + allowStateChangesEnd } from "../internal" export interface IKeyValueMap { @@ -90,7 +91,8 @@ export type IObservableMapInitialValues = // just extend Map? See also https://gist.github.com/nestharus/13b4d74f2ef4a2f4357dbd3fc23c1e54 // But: https://github.com/mobxjs/mobx/issues/1556 export class ObservableMap - implements Map, IInterceptable>, IListenable { + implements Map, IInterceptable>, IListenable +{ [$mobx] = ObservableMapMarker data_: Map> hasMap_: Map> // hasMap, not hashMap >-). @@ -110,9 +112,16 @@ export class ObservableMap 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 { diff --git a/packages/mobx/src/types/observableobject.ts b/packages/mobx/src/types/observableobject.ts index 0828577e5..129b5fbff 100644 --- a/packages/mobx/src/types/observableobject.ts +++ b/packages/mobx/src/types/observableobject.ts @@ -46,7 +46,8 @@ import { getAdministration, getDebugName, objectPrototype, - MakeResult + MakeResult, + checkIfStateModificationsAreAllowed } from "../internal" const descriptorCache = Object.create(null) @@ -314,6 +315,7 @@ export class ObservableObjectAdministration descriptor: PropertyDescriptor, proxyTrap: boolean = false ): boolean | null { + checkIfStateModificationsAreAllowed(this.keysAtom_) try { startBatch() @@ -368,6 +370,7 @@ export class ObservableObjectAdministration enhancer: IEnhancer, proxyTrap: boolean = false ): boolean | null { + checkIfStateModificationsAreAllowed(this.keysAtom_) try { startBatch() @@ -432,6 +435,7 @@ export class ObservableObjectAdministration options: IComputedValueOptions, proxyTrap: boolean = false ): boolean | null { + checkIfStateModificationsAreAllowed(this.keysAtom_) try { startBatch() @@ -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 diff --git a/packages/mobx/src/types/observableset.ts b/packages/mobx/src/types/observableset.ts index 16c797b4f..ce5d38653 100644 --- a/packages/mobx/src/types/observableset.ts +++ b/packages/mobx/src/types/observableset.ts @@ -27,7 +27,10 @@ import { DELETE, ADD, die, - isFunction + isFunction, + allowStateChangesStart, + globalState, + allowStateChangesEnd } from "../internal" const ObservableSetMarker = {} @@ -82,7 +85,14 @@ export class ObservableSet implements Set, IInterceptable 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) + } } } From e9cc9c3f122210a32beb464effe0213090a65dba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 12 Aug 2023 15:57:44 +0200 Subject: [PATCH 3/9] batchId --- .../useAsObservableSource.deprecated.test.tsx | 13 +++-- .../__tests__/useAsObservableSource.test.tsx | 13 +++-- .../__tests__/useLocalObservable.test.tsx | 8 +-- .../useLocalStore.deprecated.test.tsx | 9 ++-- packages/mobx-react/__tests__/hooks.test.tsx | 5 +- .../__tests__/v5/base/extendObservable.js | 16 +++++- .../mobx/__tests__/v5/base/observables.js | 22 ++++++++ packages/mobx/src/api/extendobservable.ts | 20 ++------ packages/mobx/src/api/makeObservable.ts | 51 +++++++------------ packages/mobx/src/api/observable.ts | 17 ++++--- packages/mobx/src/core/atom.ts | 11 +++- packages/mobx/src/core/globalstate.ts | 11 ++-- packages/mobx/src/core/observable.ts | 6 +++ .../mobx/src/types/legacyobservablearray.ts | 32 +++++------- packages/mobx/src/types/observablearray.ts | 26 ++++------ packages/mobx/src/types/observablemap.ts | 26 ++++------ packages/mobx/src/types/observableset.ts | 19 +++---- packages/mobx/src/types/type-utils.ts | 27 +++++++++- 18 files changed, 182 insertions(+), 150 deletions(-) diff --git a/packages/mobx-react-lite/__tests__/useAsObservableSource.deprecated.test.tsx b/packages/mobx-react-lite/__tests__/useAsObservableSource.deprecated.test.tsx index 46417ca4b..bf14a8d58 100644 --- a/packages/mobx-react-lite/__tests__/useAsObservableSource.deprecated.test.tsx +++ b/packages/mobx-react-lite/__tests__/useAsObservableSource.deprecated.test.tsx @@ -126,19 +126,19 @@ describe("base useAsObservableSource should work", () => { const { container } = render() expect(container.querySelector("span")!.innerHTML).toBe("10") - expect(counterRender).toBe(2) + expect(counterRender).toBe(1) act(() => { ;(container.querySelector("#inc")! as any).click() }) expect(container.querySelector("span")!.innerHTML).toBe("11") - expect(counterRender).toBe(3) + expect(counterRender).toBe(2) act(() => { ;(container.querySelector("#incmultiplier")! as any).click() }) expect(container.querySelector("span")!.innerHTML).toBe("22") - expect(counterRender).toBe(5) // TODO: should be 3 + expect(counterRender).toBe(4) // One from props, second from updating local observable (setState during render) }) }) @@ -271,7 +271,12 @@ describe("combining observer with props and stores", () => { store.x = 10 }) - expect(renderedValues).toEqual([10, 10, 15, 15, 20]) // TODO: should have one 15 less + expect(renderedValues).toEqual([ + 10, + 15, // props change + 15, // local observable change (setState during render) + 20 + ]) // TODO: re-enable this line. When debugging, the correct value is returned from render, // which is also visible with renderedValues, however, querying the dom doesn't show the correct result diff --git a/packages/mobx-react-lite/__tests__/useAsObservableSource.test.tsx b/packages/mobx-react-lite/__tests__/useAsObservableSource.test.tsx index 3ac397321..462bd9dc2 100644 --- a/packages/mobx-react-lite/__tests__/useAsObservableSource.test.tsx +++ b/packages/mobx-react-lite/__tests__/useAsObservableSource.test.tsx @@ -192,19 +192,19 @@ describe("base useAsObservableSource should work", () => { const { container } = render() expect(container.querySelector("span")!.innerHTML).toBe("10") - expect(counterRender).toBe(2) + expect(counterRender).toBe(1) act(() => { ;(container.querySelector("#inc")! as any).click() }) expect(container.querySelector("span")!.innerHTML).toBe("11") - expect(counterRender).toBe(3) + expect(counterRender).toBe(2) act(() => { ;(container.querySelector("#incmultiplier")! as any).click() }) expect(container.querySelector("span")!.innerHTML).toBe("22") - expect(counterRender).toBe(5) // TODO: should be 3 + expect(counterRender).toBe(4) // One from props, second from updating local observable with effect }) }) @@ -337,7 +337,12 @@ describe("combining observer with props and stores", () => { store.x = 10 }) - expect(renderedValues).toEqual([10, 10, 15, 15, 20]) // TODO: should have one 15 less + expect(renderedValues).toEqual([ + 10, + 15, // props change + 15, // local observable change during render (localStore.y = props.y) + 20 + ]) expect(container.querySelector("div")!.textContent).toBe("20") }) diff --git a/packages/mobx-react-lite/__tests__/useLocalObservable.test.tsx b/packages/mobx-react-lite/__tests__/useLocalObservable.test.tsx index f8f592dd4..da2dc2965 100644 --- a/packages/mobx-react-lite/__tests__/useLocalObservable.test.tsx +++ b/packages/mobx-react-lite/__tests__/useLocalObservable.test.tsx @@ -116,7 +116,7 @@ describe("is used to keep observable within component body", () => { fireEvent.click(div) expect(div.textContent).toBe("3-2") - expect(renderCount).toBe(4) + expect(renderCount).toBe(3) }) it("actions can be used", () => { @@ -418,19 +418,19 @@ describe("is used to keep observable within component body", () => { const { container } = render() expect(container.querySelector("span")!.innerHTML).toBe("10") - expect(counterRender).toBe(2) + expect(counterRender).toBe(1) act(() => { ;(container.querySelector("#inc")! as any).click() }) expect(container.querySelector("span")!.innerHTML).toBe("11") - expect(counterRender).toBe(3) + expect(counterRender).toBe(2) act(() => { ;(container.querySelector("#incmultiplier")! as any).click() }) expect(container.querySelector("span")!.innerHTML).toBe("22") - expect(counterRender).toBe(5) // TODO: should be 3 + expect(counterRender).toBe(4) // One from props, second from updating local observable with effect }) }) }) diff --git a/packages/mobx-react-lite/__tests__/useLocalStore.deprecated.test.tsx b/packages/mobx-react-lite/__tests__/useLocalStore.deprecated.test.tsx index 297bb0bf7..17fe956bf 100644 --- a/packages/mobx-react-lite/__tests__/useLocalStore.deprecated.test.tsx +++ b/packages/mobx-react-lite/__tests__/useLocalStore.deprecated.test.tsx @@ -119,8 +119,7 @@ describe("is used to keep observable within component body", () => { fireEvent.click(div) expect(div.textContent).toBe("3-2") - // though render 4 times, mobx.observable only called once - expect(renderCount).toBe(4) + expect(renderCount).toBe(3) }) it("actions can be used", () => { @@ -424,19 +423,19 @@ describe("is used to keep observable within component body", () => { const { container } = render() expect(container.querySelector("span")!.innerHTML).toBe("10") - expect(counterRender).toBe(2) + expect(counterRender).toBe(1) act(() => { ;(container.querySelector("#inc")! as any).click() }) expect(container.querySelector("span")!.innerHTML).toBe("11") - expect(counterRender).toBe(3) + expect(counterRender).toBe(2) act(() => { ;(container.querySelector("#incmultiplier")! as any).click() }) expect(container.querySelector("span")!.innerHTML).toBe("22") - expect(counterRender).toBe(5) // TODO: should be 3 + expect(counterRender).toBe(4) // One from props, second from updating source (setState during render) }) }) }) diff --git a/packages/mobx-react/__tests__/hooks.test.tsx b/packages/mobx-react/__tests__/hooks.test.tsx index 0b161a181..bc0a94cf8 100644 --- a/packages/mobx-react/__tests__/hooks.test.tsx +++ b/packages/mobx-react/__tests__/hooks.test.tsx @@ -93,10 +93,9 @@ test("computed properties result in double render when using observer instead of expect(seen).toEqual([ "parent", 0, - 0, "parent", - 2, - 2 // should contain "2" only once! But with hooks, one update is scheduled based the fact that props change, the other because the observable source changed. + 2, // props changed + 2 // observable source changed (setState during render) ]) expect(container).toHaveTextContent("2") }) diff --git a/packages/mobx/__tests__/v5/base/extendObservable.js b/packages/mobx/__tests__/v5/base/extendObservable.js index b79782106..7fdfd4b63 100644 --- a/packages/mobx/__tests__/v5/base/extendObservable.js +++ b/packages/mobx/__tests__/v5/base/extendObservable.js @@ -7,7 +7,9 @@ import { isObservableProp, isComputedProp, isAction, - extendObservable + extendObservable, + observable, + reaction } from "../../../src/mobx" test("extendObservable should work", function () { @@ -160,3 +162,15 @@ test("extendObservable should apply specified decorators", function () { expect(isAction(box.someFunc)).toBe(true) expect(box.someFunc()).toEqual(2) }) + +test("extendObservable notifies about added keys", () => { + let reactionCalled = false + const o = observable({}) + const disposeReaction = reaction( + () => Object.keys(o), + () => (reactionCalled = true) + ) + extendObservable(o, { x: 0 }) + expect(reactionCalled).toBe(true) + disposeReaction() +}) diff --git a/packages/mobx/__tests__/v5/base/observables.js b/packages/mobx/__tests__/v5/base/observables.js index b670131c7..e59a8f693 100644 --- a/packages/mobx/__tests__/v5/base/observables.js +++ b/packages/mobx/__tests__/v5/base/observables.js @@ -2388,6 +2388,15 @@ test("state version updates correctly", () => { expect(prevStateVersion).not.toBe(getGlobalState().stateVersion) }) +test("Atom.reportChanged does change state version when called from the batch the atom was created in", () => { + mobx.transaction(() => { + const prevStateVersion = getGlobalState().stateVersion + const atom = mobx.createAtom() + atom.reportChanged() + expect(prevStateVersion).toBe(getGlobalState().stateVersion) + }) +}) + test('Observables initialization does not violate `enforceActions: "always"`', () => { const consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(() => {}) @@ -2416,6 +2425,7 @@ test('Observables initialization does not violate `enforceActions: "always"`', ( check(() => mobx.makeObservable({ x: 0 }, { x: true })) check(() => new MakeAutoObservable()) check(() => mobx.makeAutoObservable({ x: 0 })) + check(() => mobx.extendObservable({}, { x: 0 })) check(() => mobx.observable(new Set([0]))) check(() => mobx.observable(new Map([[0, 0]]))) check(() => mobx.observable({ x: 0 }, { proxy: false })) @@ -2470,11 +2480,23 @@ test("state version does not update on observable creation", () => { } } + class MakeObservableSubclass extends MakeObservable { + y = 0 + constructor() { + super() + mobx.makeObservable(this, { y: true }) + } + } + check(() => mobx.observable(0)) check(() => new MakeObservable()) + // Batch is required by design - without batch reactions can be created/invoked inbetween individual constructors and they must see updated state version. + // https://github.com/mobxjs/mobx/pull/3732#discussion_r1285099080 + check(mobx.action(() => new MakeObservableSubclass())) check(() => mobx.makeObservable({ x: 0 }, { x: true })) check(() => new MakeAutoObservable()) check(() => mobx.makeAutoObservable({ x: 0 })) + check(() => mobx.extendObservable({}, { x: 0 })) check(() => mobx.observable(new Set([0]))) check(() => mobx.observable(new Map([[0, 0]]))) check(() => mobx.observable({ x: 0 }, { proxy: false })) diff --git a/packages/mobx/src/api/extendobservable.ts b/packages/mobx/src/api/extendobservable.ts index b9a2058ab..44a81f916 100644 --- a/packages/mobx/src/api/extendobservable.ts +++ b/packages/mobx/src/api/extendobservable.ts @@ -2,8 +2,6 @@ import { CreateObservableOptions, isObservableMap, AnnotationsMap, - startBatch, - endBatch, asObservableObject, isPlainObject, ObservableObjectAdministration, @@ -12,9 +10,7 @@ import { getOwnPropertyDescriptors, $mobx, ownKeys, - globalState, - allowStateChangesStart, - allowStateChangesEnd + initObservable } from "../internal" export function extendObservable( @@ -43,11 +39,8 @@ export function extendObservable( // Pull descriptors first, so we don't have to deal with props added by administration ($mobx) const descriptors = getOwnPropertyDescriptors(properties) - const adm: ObservableObjectAdministration = asObservableObject(target, options)[$mobx] - const allowStateChanges = allowStateChangesStart(true) - globalState.suppressReportChanged = true - startBatch() - try { + initObservable(() => { + const adm: ObservableObjectAdministration = asObservableObject(target, options)[$mobx] ownKeys(descriptors).forEach(key => { adm.extend_( key, @@ -56,10 +49,7 @@ export function extendObservable( !annotations ? true : key in annotations ? annotations[key] : true ) }) - } finally { - globalState.suppressReportChanged = false - allowStateChangesEnd(allowStateChanges) - endBatch() - } + }) + return target as any } diff --git a/packages/mobx/src/api/makeObservable.ts b/packages/mobx/src/api/makeObservable.ts index 51a8642e1..156161d24 100644 --- a/packages/mobx/src/api/makeObservable.ts +++ b/packages/mobx/src/api/makeObservable.ts @@ -2,8 +2,6 @@ import { $mobx, asObservableObject, AnnotationsMap, - endBatch, - startBatch, CreateObservableOptions, ObservableObjectAdministration, collectStoredAnnotations, @@ -14,9 +12,7 @@ import { extendObservable, addHiddenProp, storedAnnotationsSymbol, - globalState, - allowStateChangesStart, - allowStateChangesEnd + initObservable } from "../internal" // Hack based on https://github.com/Microsoft/TypeScript/issues/14829#issuecomment-322267089 @@ -33,11 +29,8 @@ export function makeObservable>, options?: MakeObservableOptions ): T { - const adm: ObservableObjectAdministration = asObservableObject(target, options)[$mobx] - startBatch() // TODO useless? - const allowStateChanges = allowStateChangesStart(true) - globalState.suppressReportChanged = true - try { + initObservable(() => { + const adm: ObservableObjectAdministration = asObservableObject(target, options)[$mobx] if (__DEV__ && annotations && target[storedAnnotationsSymbol]) { die( `makeObservable second arg must be nullish when using decorators. Mixing @decorator syntax with annotations is not supported.` @@ -48,11 +41,7 @@ export function makeObservable adm.make_(key, annotations![key])) - } finally { - globalState.suppressReportChanged = false - allowStateChangesEnd(allowStateChanges) - endBatch() - } + }) return target } @@ -79,22 +68,19 @@ export function makeAutoObservable { + const adm: ObservableObjectAdministration = asObservableObject(target, options)[$mobx] - // Optimization: cache keys on proto - // Assumes makeAutoObservable can be called only once per object and can't be used in subclass - if (!target[keysSymbol]) { - const proto = Object.getPrototypeOf(target) - const keys = new Set([...ownKeys(target), ...ownKeys(proto)]) - keys.delete("constructor") - keys.delete($mobx) - addHiddenProp(proto, keysSymbol, keys) - } + // Optimization: cache keys on proto + // Assumes makeAutoObservable can be called only once per object and can't be used in subclass + if (!target[keysSymbol]) { + const proto = Object.getPrototypeOf(target) + const keys = new Set([...ownKeys(target), ...ownKeys(proto)]) + keys.delete("constructor") + keys.delete($mobx) + addHiddenProp(proto, keysSymbol, keys) + } - const allowStateChanges = allowStateChangesStart(true) - globalState.suppressReportChanged = true - startBatch() // TODO useless? - try { target[keysSymbol].forEach(key => adm.make_( key, @@ -102,10 +88,7 @@ export function makeAutoObservable, options?: CreateObservableOptions ): T { - return extendObservable( - globalState.useProxies === false || options?.proxy === false - ? asObservableObject({}, options) - : asDynamicObservableObject({}, options), - props, - decorators + return initObservable(() => + extendObservable( + globalState.useProxies === false || options?.proxy === false + ? asObservableObject({}, options) + : asDynamicObservableObject({}, options), + props, + decorators + ) ) }, ref: createDecoratorAnnotation(observableRefAnnotation), diff --git a/packages/mobx/src/core/atom.ts b/packages/mobx/src/core/atom.ts index 427fd99a0..7a4741fb7 100644 --- a/packages/mobx/src/core/atom.ts +++ b/packages/mobx/src/core/atom.ts @@ -27,6 +27,7 @@ export class Atom implements IAtom { isBeingObserved_ = false observers_ = new Set() + batchId_: number diffValue_ = 0 lastAccessedBy_ = 0 lowestObserverState_ = IDerivationState_.NOT_TRACKING_ @@ -34,7 +35,9 @@ export class Atom implements IAtom { * Create a new atom. For debugging purposes it is recommended to give it a name. * The onBecomeObserved and onBecomeUnobserved callbacks can be used for resource management. */ - constructor(public name_ = __DEV__ ? "Atom@" + getNextId() : "Atom") {} + constructor(public name_ = __DEV__ ? "Atom@" + getNextId() : "Atom") { + this.batchId_ = globalState.inBatch ? globalState.batchId : NaN + } // onBecomeObservedListeners public onBOL: Set | undefined @@ -65,9 +68,13 @@ 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) { + if (globalState.inBatch && this.batchId_ === globalState.batchId) { + // Called from the same batch this atom was created in. return } + // Avoids the possibility of hitting the same globalState.batchId when it cycled through all integers (necessary?) + this.batchId_ = NaN + startBatch() propagateChanged(this) // We could update state version only at the end of batch, diff --git a/packages/mobx/src/core/globalstate.ts b/packages/mobx/src/core/globalstate.ts index d91d913e1..7ffa26006 100644 --- a/packages/mobx/src/core/globalstate.ts +++ b/packages/mobx/src/core/globalstate.ts @@ -63,6 +63,12 @@ export class MobXGlobals { */ inBatch: number = 0 + /** + * ID of the latest batch. Used to suppress reportChanged of newly created atoms. + * Note the value persists even after batch ended. + */ + batchId: number = Number.MIN_SAFE_INTEGER + /** * Observables that don't have observers anymore, and are about to be * suspended, unless somebody else accesses it in the same batch @@ -155,11 +161,6 @@ export class MobXGlobals { * Changes with each state update, used by useSyncExternalStore */ stateVersion = Number.MIN_SAFE_INTEGER - - /** - * TODO - */ - suppressReportChanged = false } let canMergeGlobalState = true diff --git a/packages/mobx/src/core/observable.ts b/packages/mobx/src/core/observable.ts index f03d5b7b6..0b17bbe42 100644 --- a/packages/mobx/src/core/observable.ts +++ b/packages/mobx/src/core/observable.ts @@ -104,6 +104,12 @@ export function queueForUnobservation(observable: IObservable) { * Avoids unnecessary recalculations. */ export function startBatch() { + if (globalState.inBatch === 0) { + globalState.batchId = + globalState.batchId < Number.MAX_SAFE_INTEGER + ? globalState.batchId + 1 + : Number.MIN_SAFE_INTEGER + } globalState.inBatch++ } diff --git a/packages/mobx/src/types/legacyobservablearray.ts b/packages/mobx/src/types/legacyobservablearray.ts index 5816bffd4..81e60dbf6 100644 --- a/packages/mobx/src/types/legacyobservablearray.ts +++ b/packages/mobx/src/types/legacyobservablearray.ts @@ -1,8 +1,6 @@ import { getNextId, addHiddenFinalProp, - allowStateChangesStart, - allowStateChangesEnd, makeIterable, addHiddenProp, ObservableArrayAdministration, @@ -12,7 +10,7 @@ import { isObservableArray, IObservableArray, defineProperty, - globalState + initObservable } from "../internal" // Bug in safari 9.* (or iOS 9 safari mobile). See #364 @@ -62,28 +60,22 @@ class LegacyObservableArray extends StubArray { owned = false ) { super() + initObservable(() => { + const adm = new ObservableArrayAdministration(name, enhancer, owned, true) + adm.proxy_ = this as any + addHiddenFinalProp(this, $mobx, adm) - const adm = new ObservableArrayAdministration(name, enhancer, owned, true) - adm.proxy_ = this as any - addHiddenFinalProp(this, $mobx, adm) - - if (initialValues && initialValues.length) { - const allowStateChanges = allowStateChangesStart(true) - globalState.suppressReportChanged = true - try { + if (initialValues && initialValues.length) { // @ts-ignore this.spliceWithArray(0, 0, initialValues) - } finally { - globalState.suppressReportChanged = false - allowStateChangesEnd(allowStateChanges) } - } - if (safariPrototypeSetterInheritanceBug) { - // Seems that Safari won't use numeric prototype setter untill any * numeric property is - // defined on the instance. After that it works fine, even if this property is deleted. - Object.defineProperty(this, "0", ENTRY_0) - } + if (safariPrototypeSetterInheritanceBug) { + // Seems that Safari won't use numeric prototype setter untill any * numeric property is + // defined on the instance. After that it works fine, even if this property is deleted. + Object.defineProperty(this, "0", ENTRY_0) + } + }) } concat(...arrays: T[][]): T[] { diff --git a/packages/mobx/src/types/observablearray.ts b/packages/mobx/src/types/observablearray.ts index 8136a7341..6c3859b3f 100644 --- a/packages/mobx/src/types/observablearray.ts +++ b/packages/mobx/src/types/observablearray.ts @@ -22,13 +22,12 @@ import { registerListener, spyReportEnd, spyReportStart, - allowStateChangesStart, - allowStateChangesEnd, assertProxies, reserveArrayBuffer, hasProp, die, - globalState + globalState, + initObservable } from "../internal" const SPLICE = "splice" @@ -406,21 +405,16 @@ export function createObservableArray( owned = false ): IObservableArray { assertProxies() - const adm = new ObservableArrayAdministration(name, enhancer, owned, false) - addHiddenFinalProp(adm.values_, $mobx, adm) - const proxy = new Proxy(adm.values_, arrayTraps) as any - adm.proxy_ = proxy - if (initialValues && initialValues.length) { - const allowStateChanges = allowStateChangesStart(true) - globalState.suppressReportChanged = true - try { + return initObservable(() => { + const adm = new ObservableArrayAdministration(name, enhancer, owned, false) + addHiddenFinalProp(adm.values_, $mobx, adm) + const proxy = new Proxy(adm.values_, arrayTraps) as any + adm.proxy_ = proxy + if (initialValues && initialValues.length) { adm.spliceWithArray_(0, 0, initialValues) - } finally { - globalState.suppressReportChanged = false - allowStateChangesEnd(allowStateChanges) } - } - return proxy + return proxy + }) } // eslint-disable-next-line diff --git a/packages/mobx/src/types/observablemap.ts b/packages/mobx/src/types/observablemap.ts index 62db97457..99bd44175 100644 --- a/packages/mobx/src/types/observablemap.ts +++ b/packages/mobx/src/types/observablemap.ts @@ -35,8 +35,7 @@ import { UPDATE, IAtom, PureSpyEvent, - allowStateChangesStart, - allowStateChangesEnd + initObservable } from "../internal" export interface IKeyValueMap { @@ -94,9 +93,9 @@ export class ObservableMap implements Map, IInterceptable>, IListenable { [$mobx] = ObservableMapMarker - data_: Map> - hasMap_: Map> // hasMap, not hashMap >-). - keysAtom_: IAtom + data_!: Map> + hasMap_!: Map> // hasMap, not hashMap >-). + keysAtom_!: IAtom interceptors_ changeListeners_ dehancer: any @@ -109,19 +108,14 @@ export class ObservableMap if (!isFunction(Map)) { die(18) } - this.keysAtom_ = createAtom(__DEV__ ? `${this.name_}.keys()` : "ObservableMap.keys()") - this.data_ = new Map() - this.hasMap_ = new Map() - if (initialData) { - const allowStateChanges = allowStateChangesStart(true) - globalState.suppressReportChanged = true - try { + initObservable(() => { + this.keysAtom_ = createAtom(__DEV__ ? `${this.name_}.keys()` : "ObservableMap.keys()") + this.data_ = new Map() + this.hasMap_ = new Map() + if (initialData) { this.merge(initialData) - } finally { - globalState.suppressReportChanged = false - allowStateChangesEnd(allowStateChanges) } - } + }) } private has_(key: K): boolean { diff --git a/packages/mobx/src/types/observableset.ts b/packages/mobx/src/types/observableset.ts index ce5d38653..bce670ba3 100644 --- a/packages/mobx/src/types/observableset.ts +++ b/packages/mobx/src/types/observableset.ts @@ -28,9 +28,7 @@ import { ADD, die, isFunction, - allowStateChangesStart, - globalState, - allowStateChangesEnd + initObservable } from "../internal" const ObservableSetMarker = {} @@ -68,7 +66,7 @@ export type ISetWillChange = export class ObservableSet implements Set, IInterceptable, IListenable { [$mobx] = ObservableSetMarker private data_: Set = new Set() - atom_: IAtom + atom_!: IAtom changeListeners_ interceptors_ dehancer: any @@ -82,18 +80,13 @@ export class ObservableSet implements Set, IInterceptable enhancer(newV, oldV, name_) - if (initialData) { - const allowStateChanges = allowStateChangesStart(true) - globalState.suppressReportChanged = true - try { + initObservable(() => { + this.atom_ = createAtom(this.name_) + if (initialData) { this.replace(initialData) - } finally { - globalState.suppressReportChanged = false - allowStateChangesEnd(allowStateChanges) } - } + }) } private dehanceValue_(value: X): X { diff --git a/packages/mobx/src/types/type-utils.ts b/packages/mobx/src/types/type-utils.ts index 161ca8bc3..14dc49ee5 100644 --- a/packages/mobx/src/types/type-utils.ts +++ b/packages/mobx/src/types/type-utils.ts @@ -10,7 +10,13 @@ import { isReaction, isObservableSet, die, - isFunction + isFunction, + allowStateChangesStart, + untrackedStart, + allowStateChangesEnd, + untrackedEnd, + startBatch, + endBatch } from "../internal" export function getAtom(thing: any, property?: PropertyKey): IDepTreeNode { @@ -92,3 +98,22 @@ export function getDebugName(thing: any, property?: string): string { } return named.name_ } + +/** + * Helper function for initializing observable structures, it applies: + * 1. allowStateChanges so we don't violate enforceActions. + * 2. untracked so we don't accidentaly subscribe to anything observable accessed during init in case the observable is created inside derivation. + * 3. batch to avoid state version updates + */ +export function initObservable(cb: () => T): T { + const derivation = untrackedStart() + const allowStateChanges = allowStateChangesStart(true) + startBatch() + try { + return cb() + } finally { + endBatch() + untrackedEnd(derivation) + allowStateChangesEnd(allowStateChanges) + } +} From 20a710694b24bd33458fa44e2acf219b53925525 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 12 Aug 2023 16:16:32 +0200 Subject: [PATCH 4/9] fix isolateGlobalState --- .changeset/spotty-emus-hear.md | 1 + .../__tests__/observer.test.tsx | 21 +++++++++++++++++++ packages/mobx-react-lite/src/useObserver.ts | 6 ++---- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/.changeset/spotty-emus-hear.md b/.changeset/spotty-emus-hear.md index 12252cb44..271a5b6ac 100644 --- a/.changeset/spotty-emus-hear.md +++ b/.changeset/spotty-emus-hear.md @@ -5,3 +5,4 @@ - fix: #3728: Observable initialization updates state version. - fix: Observable set initialization violates `enforceActions: "always"`. - fix: Changing keys of observable object does not respect `enforceActions`. +- fix: #3734: `isolateGlobalState: true` makes observer stop to react to store changes diff --git a/packages/mobx-react-lite/__tests__/observer.test.tsx b/packages/mobx-react-lite/__tests__/observer.test.tsx index b29d732bf..607e5d21d 100644 --- a/packages/mobx-react-lite/__tests__/observer.test.tsx +++ b/packages/mobx-react-lite/__tests__/observer.test.tsx @@ -1107,3 +1107,24 @@ test("StrictMode #3671", async () => { ) expect(container).toHaveTextContent("1") }) + +test("`isolateGlobalState` shouldn't break reactivity #3734", async () => { + mobx.configure({ isolateGlobalState: true }) + + const o = mobx.observable({ x: 0 }) + + const Cmp = observer(() => o.x as any) + + const { container, unmount } = render() + + expect(container).toHaveTextContent("0") + act( + mobx.action(() => { + o.x++ + }) + ) + expect(container).toHaveTextContent("1") + unmount() + + mobx._resetGlobalState() +}) diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index 77ad6241e..5c7e2ea6d 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -24,10 +24,8 @@ type ObserverAdministration = { getSnapshot: Parameters[1] } -const mobxGlobalState = _getGlobalState() - // BC -const globalStateVersionIsAvailable = typeof mobxGlobalState.stateVersion !== "undefined" +const globalStateVersionIsAvailable = typeof _getGlobalState().stateVersion !== "undefined" function createReaction(adm: ObserverAdministration) { adm.reaction = new Reaction(`observer${adm.name}`, () => { @@ -84,7 +82,7 @@ export function useObserver(render: () => T, baseComponentName: string = "obs getSnapshot() { // Do NOT access admRef here! return globalStateVersionIsAvailable - ? mobxGlobalState.stateVersion + ? _getGlobalState().stateVersion : adm.stateVersion } } From 1323692a0d893a691818dc19ce4ab6a6a56a5157 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 12 Aug 2023 16:38:58 +0200 Subject: [PATCH 5/9] tinker with timers --- ...ConcurrentModeUsingFinalizationRegistry.test.tsx | 5 ++--- .../mobx-react/__tests__/finalizationRegistry.tsx | 13 +++++-------- packages/mobx/__tests__/v5/base/typescript-tests.ts | 13 +++++++++---- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx index c8068d2ac..fb95a1d96 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx @@ -42,9 +42,8 @@ test("uncommitted components should not leak observations", async () => { ) // Allow gc to kick in in case to let finalization registry cleanup - await new Promise(resolve => setTimeout(resolve, 0)) + await Promise.resolve() gc() - await new Promise(resolve => setTimeout(resolve, 0)) // Can take a while (especially on CI) before gc actually calls the registry await waitFor( () => { @@ -55,7 +54,7 @@ test("uncommitted components should not leak observations", async () => { }, { timeout: 10_000, - interval: 200 + interval: 150 } ) }) diff --git a/packages/mobx-react/__tests__/finalizationRegistry.tsx b/packages/mobx-react/__tests__/finalizationRegistry.tsx index 76673a5b5..855e90eb9 100644 --- a/packages/mobx-react/__tests__/finalizationRegistry.tsx +++ b/packages/mobx-react/__tests__/finalizationRegistry.tsx @@ -8,12 +8,6 @@ import { observer } from "../src" afterEach(cleanup) -function sleep(time: number) { - return new Promise(res => { - setTimeout(res, time) - }) -} - // TODO remove once https://github.com/mobxjs/mobx/pull/3620 is merged. declare class WeakRef { constructor(object: T) @@ -80,9 +74,12 @@ test("should not prevent GC of uncomitted components", async () => { expect(aConstructorCount).toBe(2) expect(aMountCount).toBe(1) + await Promise.resolve() gc() - await sleep(50) - expect(firstARef!.deref()).toBeUndefined() + await waitFor(() => expect(firstARef!.deref()).toBeUndefined(), { + timeout: 10_000, + interval: 150 + }) unmount() }) diff --git a/packages/mobx/__tests__/v5/base/typescript-tests.ts b/packages/mobx/__tests__/v5/base/typescript-tests.ts index c0a344615..e7a3a510e 100644 --- a/packages/mobx/__tests__/v5/base/typescript-tests.ts +++ b/packages/mobx/__tests__/v5/base/typescript-tests.ts @@ -287,6 +287,7 @@ test("computed setter should succeed", () => { }) test("atom clock example", done => { + // TODO randomly fails, rework let ticks = 0 const time_factor = process.env.CI === "true" ? 300 : 100 // speed up / slow down tests @@ -1824,12 +1825,16 @@ test("sync when can be aborted", async () => { const x = mobx.observable.box(1) const ac = new AbortController() - mobx.when(() => x.get() === 3, () => { - fail("should abort") - }, { signal: ac.signal }) + mobx.when( + () => x.get() === 3, + () => { + fail("should abort") + }, + { signal: ac.signal } + ) ac.abort() - x.set(3); + x.set(3) }) test("it should support asyncAction as decorator (ts)", async () => { From 192e7c0db904884295e66d1b32d1e8ad10ede01f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 12 Aug 2023 16:53:45 +0200 Subject: [PATCH 6/9] initObservable: flag order --- packages/mobx/src/types/type-utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx/src/types/type-utils.ts b/packages/mobx/src/types/type-utils.ts index 14dc49ee5..e4dd9d6d1 100644 --- a/packages/mobx/src/types/type-utils.ts +++ b/packages/mobx/src/types/type-utils.ts @@ -113,7 +113,7 @@ export function initObservable(cb: () => T): T { return cb() } finally { endBatch() - untrackedEnd(derivation) allowStateChangesEnd(allowStateChanges) + untrackedEnd(derivation) } } From 8a5595f4b261e26a1a605d4b0c7b827809eed04c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 26 Aug 2023 11:20:59 +0200 Subject: [PATCH 7/9] fix test description --- packages/mobx/__tests__/v5/base/observables.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx/__tests__/v5/base/observables.js b/packages/mobx/__tests__/v5/base/observables.js index e59a8f693..86c5d7e0f 100644 --- a/packages/mobx/__tests__/v5/base/observables.js +++ b/packages/mobx/__tests__/v5/base/observables.js @@ -2388,7 +2388,7 @@ test("state version updates correctly", () => { expect(prevStateVersion).not.toBe(getGlobalState().stateVersion) }) -test("Atom.reportChanged does change state version when called from the batch the atom was created in", () => { +test("Atom.reportChanged does not change state version when called from the batch the atom was created in", () => { mobx.transaction(() => { const prevStateVersion = getGlobalState().stateVersion const atom = mobx.createAtom() From 77e617c34d924608c6540a54de433687c566827f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 26 Aug 2023 11:31:09 +0200 Subject: [PATCH 8/9] fix changeset --- .changeset/seven-poems-sneeze.md | 5 +++++ .changeset/spotty-emus-hear.md | 1 - 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 .changeset/seven-poems-sneeze.md diff --git a/.changeset/seven-poems-sneeze.md b/.changeset/seven-poems-sneeze.md new file mode 100644 index 000000000..25fa0a315 --- /dev/null +++ b/.changeset/seven-poems-sneeze.md @@ -0,0 +1,5 @@ +--- +"mobx-react-lite": patch +--- + +fix: #3734: `isolateGlobalState: true` makes observer stop to react to store changes diff --git a/.changeset/spotty-emus-hear.md b/.changeset/spotty-emus-hear.md index 271a5b6ac..12252cb44 100644 --- a/.changeset/spotty-emus-hear.md +++ b/.changeset/spotty-emus-hear.md @@ -5,4 +5,3 @@ - fix: #3728: Observable initialization updates state version. - fix: Observable set initialization violates `enforceActions: "always"`. - fix: Changing keys of observable object does not respect `enforceActions`. -- fix: #3734: `isolateGlobalState: true` makes observer stop to react to store changes From 6e81e91f0141c90176fb74770e5ebe31dd4799fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pla=C4=8Dek?= <11457665+urugator@users.noreply.github.com> Date: Sat, 26 Aug 2023 11:36:52 +0200 Subject: [PATCH 9/9] GC time tinkering --- ...trictAndConcurrentModeUsingFinalizationRegistry.test.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx index fb95a1d96..d10b894fd 100644 --- a/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx +++ b/packages/mobx-react-lite/__tests__/strictAndConcurrentModeUsingFinalizationRegistry.test.tsx @@ -42,7 +42,7 @@ test("uncommitted components should not leak observations", async () => { ) // Allow gc to kick in in case to let finalization registry cleanup - await Promise.resolve() + await new Promise(resolve => setTimeout(resolve, 100)) gc() // Can take a while (especially on CI) before gc actually calls the registry await waitFor( @@ -53,8 +53,8 @@ test("uncommitted components should not leak observations", async () => { expect(count2IsObserved).toBeFalsy() }, { - timeout: 10_000, - interval: 150 + timeout: 15_000, + interval: 200 } ) })