Skip to content

Commit

Permalink
Fixed circular dependency problem with Atoms
Browse files Browse the repository at this point in the history
  • Loading branch information
mweststrate committed Feb 26, 2018
1 parent c79f843 commit f758123
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 45 deletions.
74 changes: 42 additions & 32 deletions src/core/atom.ts
Original file line number Diff line number Diff line change
@@ -1,62 +1,74 @@
export interface IAtom extends IObservable {}
export interface IAtom extends IObservable {
reportObserved()
reportChanged()
}

/**
* Anything that can be used to _store_ state is an Atom in mobx. Atoms have two important jobs
*
* 1) detect when they are being _used_ and report this (using reportObserved). This allows mobx to make the connection between running functions and the data they used
* 2) they should notify mobx whenever they have _changed_. This way mobx can re-run any functions (derivations) that are using this atom.
*/
export class BaseAtom implements IAtom {
isPendingUnobservation = false // for effective unobserving. BaseAtom has true, for extra optimization, so its onBecomeUnobserved never gets called, because it's not needed
isBeingObserved = false
observers = []
observersIndexes = {}
export let Atom: new (name: string) => IAtom
export let isAtom: (thing: any) => thing is IAtom

export function declareAtom() {
if (Atom) return

Atom = class AtomImpl implements IAtom {
isPendingUnobservation = false // for effective unobserving. BaseAtom has true, for extra optimization, so its onBecomeUnobserved never gets called, because it's not needed
isBeingObserved = false
observers = []
observersIndexes = {}

diffValue = 0
lastAccessedBy = 0
lowestObserverState = IDerivationState.NOT_TRACKING
/**
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 = "Atom@" + getNextId()) {}
constructor(public name = "Atom@" + getNextId()) {}

public onBecomeUnobserved() {
// noop
}
public onBecomeUnobserved() {
// noop
}

public onBecomeObserved() {
/* noop */
}
public onBecomeObserved() {
/* noop */
}

/**
/**
* Invoke this method to notify mobx that your atom has been used somehow.
* Returns true if there is currently a reactive context.
*/
public reportObserved(): boolean {
return reportObserved(this)
}
public reportObserved(): boolean {
return reportObserved(this)
}

/**
/**
* Invoke this method _after_ this method has changed to signal mobx that all its observers should invalidate.
*/
public reportChanged() {
startBatch()
propagateChanged(this)
endBatch()
}
public reportChanged() {
startBatch()
propagateChanged(this)
endBatch()
}

toString() {
return this.name
toString() {
return this.name
}
}

isAtom = createInstanceofPredicate("Atom", Atom)
}

export function createAtom(
name: string,
onBecomeObservedHandler: () => void = noop,
onBecomeUnobservedHandler: () => void = noop
): IAtom {
const atom = new BaseAtom(name)
const atom = new Atom(name)
onBecomeObserved(atom, onBecomeObservedHandler)
onBecomeUnobserved(atom, onBecomeUnobservedHandler)
return atom
Expand All @@ -67,5 +79,3 @@ import { IObservable, propagateChanged, reportObserved, startBatch, endBatch } f
import { IDerivationState } from "./derivation"
import { createInstanceofPredicate, noop, getNextId } from "../utils/utils"
import { onBecomeObserved, onBecomeUnobserved } from "../api/become-observed"

export const isAtom = createInstanceofPredicate("Atom", BaseAtom)
9 changes: 1 addition & 8 deletions src/mobx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,9 @@
* - utils/ Utility stuff.
*
*/

export { IObservable, IDepTreeNode } from "./core/observable"
export { Reaction, IReactionPublic, IReactionDisposer } from "./core/reaction"
export { IDerivation, untracked, IDerivationState } from "./core/derivation"

// NOTE: For some reason, rollup's dependency tracker gets confused
// if this line is above the previous 3, and will produce out of order
// class definitions where BaseAtom is undefined, causing an error.
// It's not ideal, but for now we can just make sure this line comes after
// the ones above
export { IAtom, createAtom } from "./core/atom"

export { useStrict, IAction } from "./core/action"
Expand All @@ -35,13 +28,13 @@ export { IEqualsComparer, comparer } from "./types/comparer"
export { IModifierDescriptor, IEnhancer, isModifierDescriptor } from "./types/modifiers"
export { IInterceptable, IInterceptor } from "./types/intercept-utils"
export { IListenable } from "./types/listen-utils"

export {
IObjectWillChange,
IObjectChange,
IObservableObject,
isObservableObject
} from "./types/observableobject"

export {
IValueDidChange,
IValueWillChange,
Expand Down
6 changes: 3 additions & 3 deletions src/types/observablearray.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
addHiddenProp,
invariant
} from "../utils/utils"
import { BaseAtom } from "../core/atom"
import { Atom, IAtom } from "../core/atom"
import { checkIfStateModificationsAreAllowed } from "../core/derivation"
import {
IInterceptable,
Expand Down Expand Up @@ -147,7 +147,7 @@ if (Object.isFrozen(Array)) {

class ObservableArrayAdministration<T>
implements IInterceptable<IArrayWillChange<T> | IArrayWillSplice<T>>, IListenable {
atom: BaseAtom
atom: IAtom
values: T[] = []
lastKnownLength: number = 0
interceptors = null
Expand All @@ -161,7 +161,7 @@ class ObservableArrayAdministration<T>
public array: IObservableArray<T>,
public owned: boolean
) {
this.atom = new BaseAtom(name || "ObservableArray@" + getNextId())
this.atom = new Atom(name || "ObservableArray@" + getNextId())
this.enhancer = (newV, oldV) => enhancer(newV, oldV, name + "[..]")
}

Expand Down
6 changes: 4 additions & 2 deletions src/types/observablevalue.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { BaseAtom } from "../core/atom"
import { Atom, declareAtom } from "../core/atom"
import { checkIfStateModificationsAreAllowed } from "../core/derivation"
import {
Lambda,
Expand Down Expand Up @@ -41,7 +41,9 @@ export interface IObservableValue<T> {

declare var Symbol

export class ObservableValue<T> extends BaseAtom
declareAtom()

export class ObservableValue<T> extends Atom
implements IObservableValue<T>, IInterceptable<IValueWillChange<T>>, IListenable {
hasUnreportedChange = false
interceptors
Expand Down

4 comments on commit f758123

@trusktr
Copy link

@trusktr trusktr commented on f758123 Jul 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mweststrate Unfortunately, this technique falls apart when compiling to CommonJS, because there's no hoisting of vars or functions (beyond the module boundaries) in CommonJS. 😭

@danielkcz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trusktr Just a little correction. CommonJS has nothing to do with hoisting, that's a feature of ES2015. CommonJS is only about require and module.exports in comparison to import/export from ESM. However, since the mobx is compiled to ES5 then it indeed does not have any hoisting features. There is also mobx.es6.js distribution file. If you use that, hoisting would work and it's in CommonJS format.

@mweststrate
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trusktr sorry, I don't get what you mean by "this fails". The technique, or using MobX? If this causes a problem, please do open an issue. This was merged over a year ago, so the intent of your comment is unclear to me when posted on a commit :)

@mweststrate
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some background, read https://medium.com/visual-development/how-to-fix-nasty-circular-dependency-issues-once-and-for-all-in-javascript-typescript-a04c987cf0de?source=---------7------------------, and yes, that is specifically designed to solve the problem for commonJS, which, indeed doesn't hoist beyond module boundaries :).

Please sign in to comment.