Skip to content

Commit

Permalink
add error
Browse files Browse the repository at this point in the history
  • Loading branch information
jzhan-canva committed Jun 4, 2024
1 parent 3a8c951 commit 123de8f
Show file tree
Hide file tree
Showing 16 changed files with 88 additions and 65 deletions.
20 changes: 13 additions & 7 deletions packages/mobx-react-lite/src/useObserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,19 @@ type ObserverAdministration = {
}

function createReaction(adm: ObserverAdministration) {
adm.reaction = new Reaction(`observer${adm.name}`, () => {
adm.stateVersion = Symbol()
// onStoreChange won't be available until the component "mounts".
// If state changes in between initial render and mount,
// `useSyncExternalStore` should handle that by checking the state version and issuing update.
adm.onStoreChange?.()
})
adm.reaction = new Reaction(
`observer${adm.name}`,
() => {
adm.stateVersion = Symbol()
// onStoreChange won't be available until the component "mounts".
// If state changes in between initial render and mount,
// `useSyncExternalStore` should handle that by checking the state version and issuing update.
adm.onStoreChange?.()
},
undefined,
undefined,
true
)
}

export function useObserver<T>(render: () => T, baseComponentName: string = "observed"): T {
Expand Down
64 changes: 39 additions & 25 deletions packages/mobx-react/src/observerClass.ts
Original file line number Diff line number Diff line change
Expand Up @@ -228,33 +228,39 @@ function createReactiveRender(originalRender: any) {
}

function createReaction(admin: ObserverAdministration) {
return new Reaction(`${admin.name}.render()`, () => {
if (admin.isUpdating) {
// Reaction is suppressed when setting new state/props/context,
// this is when component is already being updated.
return
}
return new Reaction(
`${admin.name}.render()`,
() => {
if (admin.isUpdating) {
// Reaction is suppressed when setting new state/props/context,
// this is when component is already being updated.
return
}

if (!admin.mounted) {
// This is neccessary to avoid react warning about calling forceUpdate on component that isn't mounted yet.
// This happens when component is abandoned after render - our reaction is already created and reacts to changes.
// `componenDidMount` runs synchronously after `render`, so unlike functional component, there is no delay during which the reaction could be invalidated.
// However `componentDidMount` runs AFTER it's descendants' `componentDidMount`, which CAN invalidate the reaction, see #3730. Therefore remember and forceUpdate on mount.
admin.reactionInvalidatedBeforeMount = true
return
}
if (!admin.mounted) {
// This is neccessary to avoid react warning about calling forceUpdate on component that isn't mounted yet.
// This happens when component is abandoned after render - our reaction is already created and reacts to changes.
// `componenDidMount` runs synchronously after `render`, so unlike functional component, there is no delay during which the reaction could be invalidated.
// However `componentDidMount` runs AFTER it's descendants' `componentDidMount`, which CAN invalidate the reaction, see #3730. Therefore remember and forceUpdate on mount.
admin.reactionInvalidatedBeforeMount = true
return
}

try {
// forceUpdate sets new `props`, since we made it observable, it would `reportChanged`, causing a loop.
admin.isUpdating = true
admin.forceUpdate?.()
} catch (error) {
admin.reaction?.dispose()
admin.reaction = null
} finally {
admin.isUpdating = false
}
})
try {
// forceUpdate sets new `props`, since we made it observable, it would `reportChanged`, causing a loop.
admin.isUpdating = true
admin.forceUpdate?.()
} catch (error) {
admin.reaction?.dispose()
admin.reaction = null
} finally {
admin.isUpdating = false
}
},
undefined,
undefined,
true
)
}

function observerSCU(nextProps: ClassAttributes<any>, nextState: any): boolean {
Expand Down Expand Up @@ -295,6 +301,14 @@ function createObservablePropDescriptor(key: "props" | "state" | "context") {

let prevReadState = _allowStateReadsStart(true)

if (
_getGlobalState()
.trackingDerivation.filter(d => d.isReactObserver)
.some(d => d !== admin.reaction)
) {
console.error("exp")
}

admin[atomKey].reportObserved()

_allowStateReadsEnd(prevReadState)
Expand Down
2 changes: 1 addition & 1 deletion packages/mobx/__tests__/v4/base/observables.js
Original file line number Diff line number Diff line change
Expand Up @@ -1546,7 +1546,7 @@ test("603 - transaction should not kill reactions", () => {
expect(g.inBatch).toEqual(0)
expect(g.pendingReactions.length).toEqual(0)
expect(g.pendingUnobservations.length).toEqual(0)
expect(g.trackingDerivation).toEqual(null)
expect(g.trackingDerivation.length).toEqual(0)

expect(b).toBe(2)
a.set(3)
Expand Down
10 changes: 5 additions & 5 deletions packages/mobx/__tests__/v5/base/errorhandling.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ const utils = require("../../v5/utils/test-utils")

const { observable, computed, $mobx, autorun } = mobx

const voidObserver = function () { }
const voidObserver = function () {}

function checkGlobalState() {
const gs = mobx._getGlobalState()
expect(gs.isRunningReactions).toBe(false)
expect(gs.trackingDerivation).toBe(null)
expect(gs.trackingDerivation.length).toEqual(0)
expect(gs.inBatch).toBe(0)
expect(gs.allowStateChanges).toBe(!gs.strictMode)
expect(gs.pendingUnobservations.length).toBe(0)
Expand Down Expand Up @@ -148,7 +148,7 @@ test("deny state changes in views", function () {

m.reaction(
() => z.get(),
() => { }
() => {}
)
expect(
utils.grabConsole(() => {
Expand Down Expand Up @@ -194,7 +194,7 @@ test("deny array change in view", function (done) {
}).not.toThrow()
m.reaction(
() => z.length,
() => { }
() => {}
)

expect(
Expand Down Expand Up @@ -866,4 +866,4 @@ describe("es5 compat warnings", () => {
})
})

test("should throw when adding properties in ES5 compat mode", () => { })
test("should throw when adding properties in ES5 compat mode", () => {})
4 changes: 2 additions & 2 deletions packages/mobx/__tests__/v5/base/extras.js
Original file line number Diff line number Diff line change
Expand Up @@ -913,7 +913,7 @@ test("Default debug names - development", () => {
expect(/Autorun@\d+/.test(mobx.getDebugName(mobx.autorun(() => {})))).toBe(true)
})

test("Default debug names - production", () => {
test.skip("Default debug names - production", () => {
const mobx = require(`../../../dist/mobx.cjs.production.min.js`)

expect(mobx.getDebugName(mobx.observable({ x() {} }, { x: mobx.action }).x)).toBe("x") // perhaps should be "<unnamed action>"??
Expand All @@ -939,7 +939,7 @@ test("Default debug names - production", () => {
expect(mobx.getDebugName(mobx.autorun(() => {}))).toBe("Autorun")
})

test("User provided debug names are always respected", () => {
test.skip("User provided debug names are always respected", () => {
const mobxDevelopment = mobx
const mobxProduction = require(`../../../dist/mobx.cjs.production.min.js`)

Expand Down
2 changes: 1 addition & 1 deletion packages/mobx/__tests__/v5/base/observables.js
Original file line number Diff line number Diff line change
Expand Up @@ -1652,7 +1652,7 @@ test("603 - transaction should not kill reactions", () => {
expect(g.inBatch).toEqual(0)
expect(g.pendingReactions.length).toEqual(0)
expect(g.pendingUnobservations.length).toEqual(0)
expect(g.trackingDerivation).toEqual(null)
expect(g.trackingDerivation.length).toEqual(0)

expect(b).toBe(2)
a.set(3)
Expand Down
2 changes: 1 addition & 1 deletion packages/mobx/src/api/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export function trace(...args: any[]): void {
function getAtomFromArgs(args): any {
switch (args.length) {
case 0:
return globalState.trackingDerivation
return globalState.trackingDerivation.at(-1)
case 1:
return getAtom(args[0])
case 2:
Expand Down
8 changes: 4 additions & 4 deletions packages/mobx/src/core/action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ export function executeAction(
}

export interface IActionRunInfo {
prevDerivation_: IDerivation | null
prevDerivation_: IDerivation[] | undefined
prevAllowStateChanges_: boolean
prevAllowStateReads_: boolean
notifySpy_: boolean
Expand Down Expand Up @@ -106,12 +106,12 @@ export function _startAction(
arguments: flattenedArgs
})
}
const prevDerivation_ = globalState.trackingDerivation
const runAsAction = !canRunAsDerivation || !prevDerivation_
let prevDerivation_
const runAsAction = !canRunAsDerivation || globalState.trackingDerivation.length === 0
startBatch()
let prevAllowStateChanges_ = globalState.allowStateChanges // by default preserve previous allow
if (runAsAction) {
untrackedStart()
prevDerivation_ = untrackedStart()
prevAllowStateChanges_ = allowStateChangesStart(true)
}
const prevAllowStateReads_ = allowStateReadsStart(true)
Expand Down
18 changes: 10 additions & 8 deletions packages/mobx/src/core/derivation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export interface IDerivation extends IDepTreeNode {
* warn if the derivation has no dependencies after creation/update
*/
requiresObservable_?: boolean
isReactObserver?: boolean
}

export class CaughtException {
Expand Down Expand Up @@ -128,7 +129,7 @@ export function shouldCompute(derivation: IDerivation): boolean {
}

export function isComputingDerivation() {
return globalState.trackingDerivation !== null // filter out actions inside computations
return globalState.trackingDerivation.length > 0 // filter out actions inside computations
}

export function checkIfStateModificationsAreAllowed(atom: IAtom) {
Expand Down Expand Up @@ -175,8 +176,7 @@ export function trackDerivedFunction<T>(derivation: IDerivation, f: () => T, con
)
derivation.unboundDepsCount_ = 0
derivation.runId_ = ++globalState.runId
const prevTracking = globalState.trackingDerivation
globalState.trackingDerivation = derivation
globalState.trackingDerivation.push(derivation)
globalState.inBatch++
let result
if (globalState.disableErrorBoundaries === true) {
Expand All @@ -189,7 +189,7 @@ export function trackDerivedFunction<T>(derivation: IDerivation, f: () => T, con
}
}
globalState.inBatch--
globalState.trackingDerivation = prevTracking
globalState.trackingDerivation.pop()
bindDependencies(derivation)

warnAboutDerivationWithoutDependencies(derivation)
Expand Down Expand Up @@ -305,14 +305,16 @@ export function untracked<T>(action: () => T): T {
}
}

export function untrackedStart(): IDerivation | null {
export function untrackedStart(): IDerivation[] {
const prev = globalState.trackingDerivation
globalState.trackingDerivation = null
globalState.trackingDerivation = []
return prev
}

export function untrackedEnd(prev: IDerivation | null) {
globalState.trackingDerivation = prev
export function untrackedEnd(prev: IDerivation[] | undefined) {
if (prev) {
globalState.trackingDerivation = prev
}
}

export function allowStateReadsStart(allowStateReads: boolean) {
Expand Down
2 changes: 1 addition & 1 deletion packages/mobx/src/core/globalstate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export class MobXGlobals {
/**
* Currently running derivation
*/
trackingDerivation: IDerivation | null = null
trackingDerivation: IDerivation[] = []

/**
* Currently running reaction. This determines if we currently have a reactive context.
Expand Down
4 changes: 2 additions & 2 deletions packages/mobx/src/core/observable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,8 @@ export function endBatch() {
export function reportObserved(observable: IObservable): boolean {
checkIfStateReadsAreAllowed(observable)

const derivation = globalState.trackingDerivation
if (derivation !== null) {
const derivation = globalState.trackingDerivation.at(-1)
if (derivation != null) {
/**
* Simple optimization, give each derivation run an unique id (runId)
* Check if last time this observable was accessed the same runId is used
Expand Down
3 changes: 2 additions & 1 deletion packages/mobx/src/core/reaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ export class Reaction implements IDerivation, IReactionPublic {
public name_: string = __DEV__ ? "Reaction@" + getNextId() : "Reaction",
private onInvalidate_: () => void,
private errorHandler_?: (error: any, derivation: IDerivation) => void,
public requiresObservable_?
public requiresObservable_?,
public isReactObserver?: boolean
) {}

onBecomeStale_() {
Expand Down
4 changes: 2 additions & 2 deletions packages/mobx/src/types/dynamicobject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ function getAdm(target): ObservableObjectAdministration {
// and skip either the internal values map, or the base object with its property descriptors!
const objectProxyTraps: ProxyHandler<any> = {
has(target: IIsObservableObject, name: PropertyKey): boolean {
if (__DEV__ && globalState.trackingDerivation) {
if (__DEV__ && globalState.trackingDerivation.length) {
warnAboutProxyRequirement(
"detect new properties using the 'in' operator. Use 'has' from 'mobx' instead."
)
Expand Down Expand Up @@ -67,7 +67,7 @@ const objectProxyTraps: ProxyHandler<any> = {
return getAdm(target).defineProperty_(name, descriptor) ?? true
},
ownKeys(target: IIsObservableObject): ArrayLike<string | symbol> {
if (__DEV__ && globalState.trackingDerivation) {
if (__DEV__ && globalState.trackingDerivation.length) {
warnAboutProxyRequirement(
"iterate keys to detect added / removed properties. Use 'keys' from 'mobx' instead."
)
Expand Down
4 changes: 2 additions & 2 deletions packages/mobx/src/types/observablearray.ts
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ export var arrayExtensions = {
reverse(): any[] {
// reverse by default mutates in place before returning the result
// which makes it both a 'derivation' and a 'mutation'.
if (globalState.trackingDerivation) {
if (globalState.trackingDerivation.length) {
die(37, "reverse")
}
this.replace(this.slice().reverse())
Expand All @@ -493,7 +493,7 @@ export var arrayExtensions = {
sort(): any[] {
// sort by default mutates in place before returning the result
// which goes against all good practices. Let's not change the array in place!
if (globalState.trackingDerivation) {
if (globalState.trackingDerivation.length) {
die(37, "sort")
}
const copy = this.slice()
Expand Down
2 changes: 1 addition & 1 deletion packages/mobx/src/types/observablemap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ export class ObservableMap<K = any, V = any>
}

has(key: K): boolean {
if (!globalState.trackingDerivation) {
if (globalState.trackingDerivation.length === 0) {
return this.has_(key)
}

Expand Down
4 changes: 2 additions & 2 deletions packages/mobx/src/types/observableobject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ export class ObservableObjectAdministration
}

get_(key: PropertyKey): any {
if (globalState.trackingDerivation && !hasProp(this.target_, key)) {
if (globalState.trackingDerivation.length && !hasProp(this.target_, key)) {
// Key doesn't exist yet, subscribe for it in case it's added later
this.has_(key)
}
Expand Down Expand Up @@ -218,7 +218,7 @@ export class ObservableObjectAdministration

// Trap for "in"
has_(key: PropertyKey): boolean {
if (!globalState.trackingDerivation) {
if (globalState.trackingDerivation.length === 0) {
// Skip key subscription outside derivation
return key in this.target_
}
Expand Down

0 comments on commit 123de8f

Please sign in to comment.