From af64a0a3abf2a7b77ee274d12229ee732856b7b2 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Sat, 25 Nov 2023 15:09:52 +0000 Subject: [PATCH] State version cleanup (#3764) * fix: disable global state version usage to avoid footgun with fresh props not propagationg, fixes #3728 * chore: Added changeset * chore: cleanup global state version --- packages/mobx-react-lite/src/useObserver.ts | 5 ++- .../mobx/__tests__/v5/base/observables.js | 37 ------------------- packages/mobx/src/core/atom.ts | 19 +--------- packages/mobx/src/core/globalstate.ts | 11 ------ packages/mobx/src/core/observable.ts | 6 --- 5 files changed, 5 insertions(+), 73 deletions(-) diff --git a/packages/mobx-react-lite/src/useObserver.ts b/packages/mobx-react-lite/src/useObserver.ts index 8bc07b360..bacc48a10 100644 --- a/packages/mobx-react-lite/src/useObserver.ts +++ b/packages/mobx-react-lite/src/useObserver.ts @@ -13,10 +13,11 @@ const getServerSnapshot = () => {} type ObserverAdministration = { reaction: Reaction | null // also serves as disposed flag onStoreChange: Function | null // also serves as mounted flag - // BC: we will use local state version if global isn't available. - // It should behave as previous implementation - tearing is still present, + // stateVersion that 'ticks' for every time the reaction fires + // tearing is still present, // because there is no cross component synchronization, // but we can use `useSyncExternalStore` API. + // TODO: optimize to use number? stateVersion: any name: string // These don't depend on state/props, therefore we can keep them here instead of `useCallback` diff --git a/packages/mobx/__tests__/v5/base/observables.js b/packages/mobx/__tests__/v5/base/observables.js index 40e28949a..24d229ac1 100644 --- a/packages/mobx/__tests__/v5/base/observables.js +++ b/packages/mobx/__tests__/v5/base/observables.js @@ -2361,43 +2361,6 @@ describe("`requiresObservable` takes precedence over global `reactionRequiresObs }) }) -test("state version updates correctly", () => { - // This test was designed around the idea of updating version only at the end of batch, - // which is NOT an implementation we've settled on, but the test is still valid. - - // This test demonstrates that the version is correctly updated with each state mutations: - // 1. Even without wrapping mutation in batch explicitely. - // 2. Even in self-invoking recursive derivation. - const o = mobx.observable({ x: 0 }) - let prevStateVersion - - const disposeAutorun = mobx.autorun(() => { - if (o.x === 5) { - disposeAutorun() - return - } - const currentStateVersion = getGlobalState().stateVersion - expect(prevStateVersion).not.toBe(currentStateVersion) - prevStateVersion = currentStateVersion - o.x++ - }) - - // expect(o.x).toBe(4) is 1? - prevStateVersion = getGlobalState().stateVersion - o.x++ - expect(o.x).toBe(5) - expect(prevStateVersion).not.toBe(getGlobalState().stateVersion) -}) - -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() - atom.reportChanged() - expect(prevStateVersion).toBe(getGlobalState().stateVersion) - }) -}) - test('Observables initialization does not violate `enforceActions: "always"`', () => { const consoleWarnSpy = jest.spyOn(console, "warn").mockImplementation(() => {}) diff --git a/packages/mobx/src/core/atom.ts b/packages/mobx/src/core/atom.ts index 673e7687a..29ce0213d 100644 --- a/packages/mobx/src/core/atom.ts +++ b/packages/mobx/src/core/atom.ts @@ -11,8 +11,7 @@ import { propagateChanged, reportObserved, startBatch, - Lambda, - globalState + Lambda } from "../internal" export const $mobx = Symbol("mobx administration") @@ -27,7 +26,6 @@ export class Atom implements IAtom { isBeingObserved_ = false observers_ = new Set() - batchId_: number diffValue_ = 0 lastAccessedBy_ = 0 lowestObserverState_ = IDerivationState_.NOT_TRACKING_ @@ -35,9 +33,7 @@ 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") { - this.batchId_ = globalState.inBatch ? globalState.batchId : NaN - } + constructor(public name_ = __DEV__ ? "Atom@" + getNextId() : "Atom") {} // onBecomeObservedListeners public onBOL: Set | undefined @@ -68,17 +64,6 @@ 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.inBatch || this.batchId_ !== globalState.batchId) { - // We could update state version only at the end of batch, - // but we would still have to switch some global flag here to signal a change. - globalState.stateVersion = - globalState.stateVersion < Number.MAX_SAFE_INTEGER - ? globalState.stateVersion + 1 - : Number.MIN_SAFE_INTEGER - // Avoids the possibility of hitting the same globalState.batchId when it cycled through all integers (necessary?) - this.batchId_ = NaN - } - startBatch() propagateChanged(this) endBatch() diff --git a/packages/mobx/src/core/globalstate.ts b/packages/mobx/src/core/globalstate.ts index 7ffa26006..35de9dfea 100644 --- a/packages/mobx/src/core/globalstate.ts +++ b/packages/mobx/src/core/globalstate.ts @@ -63,12 +63,6 @@ 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 @@ -156,11 +150,6 @@ export class MobXGlobals { * configurable: true */ safeDescriptors = true - - /** - * Changes with each state update, used by useSyncExternalStore - */ - stateVersion = Number.MIN_SAFE_INTEGER } let canMergeGlobalState = true diff --git a/packages/mobx/src/core/observable.ts b/packages/mobx/src/core/observable.ts index 0b17bbe42..f03d5b7b6 100644 --- a/packages/mobx/src/core/observable.ts +++ b/packages/mobx/src/core/observable.ts @@ -104,12 +104,6 @@ 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++ }