From 00aa71610ab2047d5a9b628446a57eb33eb1e235 Mon Sep 17 00:00:00 2001 From: Artur Eshenbrener Date: Thu, 7 Jun 2018 17:41:07 +0300 Subject: [PATCH 1/2] Make props & state observable in constructor & componentWillMount Moved instrumentation of state & props to `observer` decorator. Closes #478 --- src/observer.js | 69 +++++++++++++++++++++++--------------------- test/issue21.test.js | 52 +++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 33 deletions(-) diff --git a/src/observer.js b/src/observer.js index 7e192bac..acec3a5f 100644 --- a/src/observer.js +++ b/src/observer.js @@ -20,6 +20,9 @@ let warnedAboutObserverInjectDeprecation = false export const componentByNodeRegistry = typeof WeakMap !== "undefined" ? new WeakMap() : undefined export const renderReporter = new EventEmitter() +const skipRenderKey = Symbol("skipRender") +const isForcingUpdateKey = Symbol("isForcingUpdate") + function findDOMNode(component) { if (baseFindDOMNode) { try { @@ -116,29 +119,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 @@ -177,17 +157,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 + 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") + this[isForcingUpdateKey] = false // wire up reactive render const baseRender = render.bind(this) @@ -206,11 +181,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) + this[isForcingUpdateKey] = true + if (!this[skipRenderKey]) Component.prototype.forceUpdate.call(this) hasError = false } finally { - isForcingUpdate = false + this[isForcingUpdateKey] = false if (hasError) reaction.dispose() } } @@ -273,6 +248,32 @@ const reactiveMixin = { } } +function makeObservableProp(target, propName) { + const valueHolderKey = Symbol(propName + " value holder") + const atomHolderKey = Symbol(propName + " atom holder") + function getAtom() { + return this[atomHolderKey] || (this[atomHolderKey] = createAtom("reactive " + propName)) + } + 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)) { + this[valueHolderKey] = v + this[skipRenderKey] = true + getAtom.call(this).reportChanged() + this[skipRenderKey] = false + } else { + this[valueHolderKey] = v + } + } + }) +} + /** * Observer function / decorator */ @@ -340,6 +341,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) diff --git a/test/issue21.test.js b/test/issue21.test.js index d1ace35a..81125b73 100644 --- a/test/issue21.test.js +++ b/test/issue21.test.js @@ -449,3 +449,55 @@ test("lifecycle callbacks called with correct arguments", async () => { await asyncReactDOMRender(, 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
+ } + } + ) + + await asyncReactDOMRender(, testRoot) + await asyncReactDOMRender(, testRoot) + await asyncReactDOMRender(, testRoot) + await asyncReactDOMRender(, testRoot) + expect(constructorCallsCount).toEqual(1) + expect(componentWillMountCallsCount).toEqual(1) + expect(prop1Values).toEqual(["1", "2", "3", "4"]) + expect(prop2Values).toEqual(["4", "3", "2", "1"]) +}) From 5193ad816164d20b4d00a93e22a80a7405414994 Mon Sep 17 00:00:00 2001 From: Artur Eshenbrener Date: Thu, 7 Jun 2018 22:15:44 +0300 Subject: [PATCH 2/2] Made internal props hidden --- src/observer.js | 49 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 9 deletions(-) diff --git a/src/observer.js b/src/observer.js index acec3a5f..29e90f46 100644 --- a/src/observer.js +++ b/src/observer.js @@ -20,9 +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 { @@ -157,12 +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 */ - this[skipRenderKey] = false + setHiddenProp(this, skipRenderKey, false) /** * forceUpdate will re-assign this.props. We don't want that to cause a loop, * so detect these changes */ - this[isForcingUpdateKey] = false + setHiddenProp(this, isForcingUpdateKey, false) // wire up reactive render const baseRender = render.bind(this) @@ -181,11 +209,11 @@ function makeComponentReactive(render) { // However, people also claim this migth happen during unit tests.. let hasError = true try { - this[isForcingUpdateKey] = true + setHiddenProp(this, isForcingUpdateKey, true) if (!this[skipRenderKey]) Component.prototype.forceUpdate.call(this) hasError = false } finally { - this[isForcingUpdateKey] = false + setHiddenProp(this, isForcingUpdateKey, false) if (hasError) reaction.dispose() } } @@ -252,7 +280,10 @@ function makeObservableProp(target, propName) { const valueHolderKey = Symbol(propName + " value holder") const atomHolderKey = Symbol(propName + " atom holder") function getAtom() { - return this[atomHolderKey] || (this[atomHolderKey] = createAtom("reactive " + propName)) + if (!this[atomHolderKey]) { + setHiddenProp(this, atomHolderKey, createAtom("reactive " + propName)) + } + return this[atomHolderKey] } Object.defineProperty(target, propName, { configurable: true, @@ -263,12 +294,12 @@ function makeObservableProp(target, propName) { }, set: function set(v) { if (!this[isForcingUpdateKey] && !shallowEqual(this[valueHolderKey], v)) { - this[valueHolderKey] = v - this[skipRenderKey] = true + setHiddenProp(this, valueHolderKey, v) + setHiddenProp(this, skipRenderKey, true) getAtom.call(this).reportChanged() - this[skipRenderKey] = false + setHiddenProp(this, skipRenderKey, false) } else { - this[valueHolderKey] = v + setHiddenProp(this, valueHolderKey, v) } } })