Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

Make props & state observable in constructor & componentWillMount #496

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 67 additions & 33 deletions src/observer.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,37 @@ let warnedAboutObserverInjectDeprecation = false
export const componentByNodeRegistry = typeof WeakMap !== "undefined" ? new WeakMap() : undefined
export const renderReporter = new EventEmitter()

const knownNonEnumerablePropsKey = Symbol("knownNonEnumerableProps")
const skipRenderKey = Symbol("skipRender")
const isForcingUpdateKey = Symbol("isForcingUpdate")

/**
* Helper to set `prop` to `this` as non-enumerable (hidden prop)
* @param prop
* @param value
*/
function setHiddenProp(target, prop, value) {
if (!target[knownNonEnumerablePropsKey]) {
Object.defineProperty(target, knownNonEnumerablePropsKey, {
enumerable: false,
configurable: false,
writable: false,
value: {}
})
}
if (!target[knownNonEnumerablePropsKey][prop]) {
Object.defineProperty(target, prop, {
enumerable: false,
configurable: false,
writable: true,
value: value
})
target[knownNonEnumerablePropsKey][prop] = true
} else {
target[prop] = value
}
}

function findDOMNode(component) {
if (baseFindDOMNode) {
try {
Expand Down Expand Up @@ -116,29 +147,6 @@ function is(x, y) {
function makeComponentReactive(render) {
if (isUsingStaticRendering === true) return render.call(this)

function makePropertyObservableReference(propName) {
let valueHolder = this[propName]
const atom = createAtom("reactive " + propName)
Object.defineProperty(this, propName, {
configurable: true,
enumerable: true,
get: function() {
atom.reportObserved()
return valueHolder
},
set: function set(v) {
if (!isForcingUpdate && !shallowEqual(valueHolder, v)) {
valueHolder = v
skipRender = true
atom.reportChanged()
skipRender = false
} else {
valueHolder = v
}
}
})
}

function reactiveRender() {
isRenderingPending = false
let exception = undefined
Expand Down Expand Up @@ -177,17 +185,12 @@ function makeComponentReactive(render) {
* If props are shallowly modified, react will render anyway,
* so atom.reportChanged() should not result in yet another re-render
*/
let skipRender = false
setHiddenProp(this, skipRenderKey, false)
/**
* forceUpdate will re-assign this.props. We don't want that to cause a loop,
* so detect these changes
*/
let isForcingUpdate = false

// make this.props an observable reference, see #124
makePropertyObservableReference.call(this, "props")
// make state an observable reference
makePropertyObservableReference.call(this, "state")
setHiddenProp(this, isForcingUpdateKey, false)

// wire up reactive render
const baseRender = render.bind(this)
Expand All @@ -206,11 +209,11 @@ function makeComponentReactive(render) {
// However, people also claim this migth happen during unit tests..
let hasError = true
try {
isForcingUpdate = true
if (!skipRender) Component.prototype.forceUpdate.call(this)
setHiddenProp(this, isForcingUpdateKey, true)
if (!this[skipRenderKey]) Component.prototype.forceUpdate.call(this)
hasError = false
} finally {
isForcingUpdate = false
setHiddenProp(this, isForcingUpdateKey, false)
if (hasError) reaction.dispose()
}
}
Expand Down Expand Up @@ -273,6 +276,35 @@ const reactiveMixin = {
}
}

function makeObservableProp(target, propName) {
const valueHolderKey = Symbol(propName + " value holder")
const atomHolderKey = Symbol(propName + " atom holder")
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering whether using Symbols is a breaking change. So far we didn't depend on them, but I think React itself already requires Symbol or a polyfill of it? (Since mobx-react 5 can still be used with mobx 4 so should work on any ES 5 env)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will check this out, maybe there is a way to do this without symbols.

Copy link
Contributor

Choose a reason for hiding this comment

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

The support looks pretty good to me: https://caniuse.com/#search=symbol

Copy link
Member

@mweststrate mweststrate Jun 7, 2018

Choose a reason for hiding this comment

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

without symbols it could be done by putting an object under some ugly key. Symbols is preferred though :) You could always create a super naive polyfill like: function Symbol(name) { return ``$mobxReactProp${symbolname}${Math.random()}`` }. But since react itself already requires quite some polyfills (link, so I think we can bet on Symbol to be present. And otherwise fix it if somebody reports it

function getAtom() {
if (!this[atomHolderKey]) {
setHiddenProp(this, atomHolderKey, createAtom("reactive " + propName))
}
return this[atomHolderKey]
}
Object.defineProperty(target, propName, {
configurable: true,
enumerable: true,
get: function() {
getAtom.call(this).reportObserved()
return this[valueHolderKey]
},
set: function set(v) {
if (!this[isForcingUpdateKey] && !shallowEqual(this[valueHolderKey], v)) {
setHiddenProp(this, valueHolderKey, v)
setHiddenProp(this, skipRenderKey, true)
getAtom.call(this).reportChanged()
setHiddenProp(this, skipRenderKey, false)
} else {
setHiddenProp(this, valueHolderKey, v)
}
}
})
}

/**
* Observer function / decorator
*/
Expand Down Expand Up @@ -340,6 +372,8 @@ export function observer(arg1, arg2) {
const target = componentClass.prototype || componentClass
mixinLifecycleEvents(target)
componentClass.isMobXReactObserver = true
makeObservableProp(target, "props")
makeObservableProp(target, "state")
const baseRender = target.render
target.render = function() {
return makeComponentReactive.call(this, baseRender)
Expand Down
52 changes: 52 additions & 0 deletions test/issue21.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -449,3 +449,55 @@ test("lifecycle callbacks called with correct arguments", async () => {
await asyncReactDOMRender(<Root />, testRoot)
testRoot.querySelector("#testButton").click()
})

test("verify props are reactive in componentWillMount and constructor", async () => {
const prop1Values = []
const prop2Values = []
let componentWillMountCallsCount = 0
let constructorCallsCount = 0

const Component = observer(
class Component extends React.Component {
constructor(props, context) {
super(props, context)
constructorCallsCount++
this.disposer1 = mobx.reaction(
() => this.props.prop1,
prop => prop1Values.push(prop),
{
fireImmediately: true
}
)
}

componentWillMount() {
componentWillMountCallsCount++
this.disposer2 = mobx.reaction(
() => this.props.prop2,
prop => prop2Values.push(prop),
{
fireImmediately: true
}
)
}

componentWillUnmount() {
this.disposer1()
this.disposer2()
}

render() {
return <div />
}
}
)

await asyncReactDOMRender(<Component prop1="1" prop2="4" />, testRoot)
await asyncReactDOMRender(<Component prop1="2" prop2="3" />, testRoot)
await asyncReactDOMRender(<Component prop1="3" prop2="2" />, testRoot)
await asyncReactDOMRender(<Component prop1="4" prop2="1" />, testRoot)
expect(constructorCallsCount).toEqual(1)
expect(componentWillMountCallsCount).toEqual(1)
expect(prop1Values).toEqual(["1", "2", "3", "4"])
expect(prop2Values).toEqual(["4", "3", "2", "1"])
})