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

Author: mweststrate

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`

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(() => {})
 

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,17 +26,14 @@ export class Atom implements IAtom {
     isBeingObserved_ = false
     observers_ = new Set<IDerivation>()
 
-    batchId_: number
     diffValue_ = 0
     lastAccessedBy_ = 0
     lowestObserverState_ = IDerivationState_.NOT_TRACKING_
     /**
      * 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<Lambda> | 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()

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

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++
 }