Skip to content

Commit

Permalink
State version cleanup (#3764)
Browse files Browse the repository at this point in the history
* fix: disable global state version usage to avoid footgun with fresh props not propagationg, fixes #3728

* chore: Added changeset

* chore: cleanup global state version
  • Loading branch information
mweststrate committed Nov 25, 2023
1 parent a2db19e commit af64a0a
Show file tree
Hide file tree
Showing 5 changed files with 5 additions and 73 deletions.
5 changes: 3 additions & 2 deletions packages/mobx-react-lite/src/useObserver.ts
Expand Up @@ -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`
Expand Down
37 changes: 0 additions & 37 deletions packages/mobx/__tests__/v5/base/observables.js
Expand Up @@ -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(() => {})

Expand Down
19 changes: 2 additions & 17 deletions packages/mobx/src/core/atom.ts
Expand Up @@ -11,8 +11,7 @@ import {
propagateChanged,
reportObserved,
startBatch,
Lambda,
globalState
Lambda
} from "../internal"

export const $mobx = Symbol("mobx administration")
Expand All @@ -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
Expand Down Expand Up @@ -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()
Expand Down
11 changes: 0 additions & 11 deletions packages/mobx/src/core/globalstate.ts
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 0 additions & 6 deletions packages/mobx/src/core/observable.ts
Expand Up @@ -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++
}

Expand Down

0 comments on commit af64a0a

Please sign in to comment.