From c00a660b9115e1f39069f622106b8bccb53bbac2 Mon Sep 17 00:00:00 2001 From: Brandon Dail Date: Thu, 10 Nov 2016 13:08:33 -0600 Subject: [PATCH 1/5] Use _hostContainerInfo to track test renderer options The transaction is not available when unmounting or updating the instance, so we track it using _hostContainerInfo --- scripts/fiber/tests-failing.txt | 3 ++ scripts/fiber/tests-passing.txt | 1 + src/renderers/testing/ReactTestMount.js | 10 ++++-- src/renderers/testing/ReactTestRenderer.js | 12 +++++-- .../__tests__/ReactTestRenderer-test.js | 35 +++++++++++++++++++ 5 files changed, 55 insertions(+), 6 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index a41b7230a1eca..fb555d55ceaf6 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -589,6 +589,9 @@ src/renderers/shared/stack/reconciler/__tests__/ReactUpdates-test.js src/renderers/shared/stack/reconciler/__tests__/refs-test.js * Should increase refs with an increase in divs +src/renderers/testing/__tests__/ReactTestRenderer-test.js +* supports updates when using refs + src/shared/utils/__tests__/traverseAllChildren-test.js * should warn for using maps as children with owner info diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index b554584d0163e..8c2b937a3309f 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1206,6 +1206,7 @@ src/renderers/testing/__tests__/ReactTestRenderer-test.js * gives a ref to native components * warns correctly for refs on SFCs * allows an optional createNodeMock function +* supports unmounting when using refs * supports error boundaries src/shared/utils/__tests__/KeyEscapeUtils-test.js diff --git a/src/renderers/testing/ReactTestMount.js b/src/renderers/testing/ReactTestMount.js index d1dcdfeb41865..9bad563445cb0 100644 --- a/src/renderers/testing/ReactTestMount.js +++ b/src/renderers/testing/ReactTestMount.js @@ -56,12 +56,14 @@ TopLevelWrapper.isReactTopLevelWrapper = true; function mountComponentIntoNode( componentInstance, transaction, + hostParent, + hostContainerInfo ) { var image = ReactReconciler.mountComponent( componentInstance, transaction, null, - null, + hostContainerInfo, emptyObject ); componentInstance._renderedComponent._topLevelWrapper = componentInstance; @@ -79,12 +81,14 @@ function batchedMountComponentIntoNode( componentInstance, options, ) { - var transaction = ReactUpdates.ReactReconcileTransaction.getPooled(options); + var transaction = ReactUpdates.ReactReconcileTransaction.getPooled(true); var image = transaction.perform( mountComponentIntoNode, null, componentInstance, transaction, + null, + options ); ReactUpdates.ReactReconcileTransaction.release(transaction); return image; @@ -96,7 +100,7 @@ var ReactTestInstance = function(component) { ReactTestInstance.prototype.getInstance = function() { return this._component._renderedComponent.getPublicInstance(); }; -ReactTestInstance.prototype.update = function(nextElement) { +ReactTestInstance.prototype.update = function(nextElement, options) { invariant( this._component, "ReactTestRenderer: .update() can't be called after unmount." diff --git a/src/renderers/testing/ReactTestRenderer.js b/src/renderers/testing/ReactTestRenderer.js index 47184fea0c1bf..399e5024d6125 100644 --- a/src/renderers/testing/ReactTestRenderer.js +++ b/src/renderers/testing/ReactTestRenderer.js @@ -53,20 +53,23 @@ class ReactTestComponent { _currentElement: ReactElement; _renderedChildren: null | Object; _topLevelWrapper: null | ReactInstance; + _hostContainerInfo: null | Object; constructor(element: ReactElement) { this._currentElement = element; this._renderedChildren = null; this._topLevelWrapper = null; + this._hostContainerInfo = null; } mountComponent( transaction: ReactTestReconcileTransaction, nativeParent: null | ReactTestComponent, - nativeContainerInfo: ?null, + hostContainerInfo: null | Object, context: Object, ) { var element = this._currentElement; + this._hostContainerInfo = hostContainerInfo; // $FlowFixMe https://github.com/facebook/flow/issues/1805 this.mountChildren(element.props.children, transaction, context); } @@ -83,8 +86,11 @@ class ReactTestComponent { getPublicInstance(transaction: ReactTestReconcileTransaction): Object { var element = this._currentElement; - var options = transaction.getTestOptions(); - return options.createNodeMock(element); + var options = this._hostContainerInfo; + if (options && options.createNodeMock) { + return options.createNodeMock(element); + } + return {}; } toJSON(): ReactTestRendererJSON { diff --git a/src/renderers/testing/__tests__/ReactTestRenderer-test.js b/src/renderers/testing/__tests__/ReactTestRenderer-test.js index 7ca6c201396ce..a9e47a6777ca7 100644 --- a/src/renderers/testing/__tests__/ReactTestRenderer-test.js +++ b/src/renderers/testing/__tests__/ReactTestRenderer-test.js @@ -306,6 +306,41 @@ describe('ReactTestRenderer', () => { ]); }); + it('supports unmounting when using refs', () => { + class Foo extends React.Component { + render() { + return
; + } + } + const inst = ReactTestRenderer.create( + , + { createNodeMock: () => 'foo' } + ); + expect(() => inst.unmount()).not.toThrow(); + }); + + it('supports updates when using refs', () => { + const log = []; + const createNodeMock = element => { + log.push(element.type); + return element.type; + }; + class Foo extends React.Component { + render() { + return this.props.useDiv + ?
+ : ; + } + } + const inst = ReactTestRenderer.create( + , + { createNodeMock } + ); + inst.update(); + // It's called with 'div' twice (mounting and unmounting) + expect(log).toEqual(['div', 'div', 'span']); + }); + it('supports error boundaries', () => { var log = []; class Angry extends React.Component { From 70f274fc4e1ff42278563297a9417106825abcae Mon Sep 17 00:00:00 2001 From: Brandon Dail Date: Thu, 10 Nov 2016 13:57:29 -0600 Subject: [PATCH 2/5] Throw if hostContainerInfo is not populated in getPublicInstance --- src/renderers/testing/ReactTestRenderer.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/renderers/testing/ReactTestRenderer.js b/src/renderers/testing/ReactTestRenderer.js index 399e5024d6125..7b180f8d49690 100644 --- a/src/renderers/testing/ReactTestRenderer.js +++ b/src/renderers/testing/ReactTestRenderer.js @@ -23,6 +23,7 @@ var ReactTestReconcileTransaction = require('ReactTestReconcileTransaction'); var ReactUpdates = require('ReactUpdates'); var ReactTestTextComponent = require('ReactTestTextComponent'); var ReactTestEmptyComponent = require('ReactTestEmptyComponent'); +var invariant = require('invariant'); import type { ReactElement } from 'ReactElementType'; import type { ReactInstance } from 'ReactInstanceType'; @@ -65,7 +66,7 @@ class ReactTestComponent { mountComponent( transaction: ReactTestReconcileTransaction, nativeParent: null | ReactTestComponent, - hostContainerInfo: null | Object, + hostContainerInfo: Object, context: Object, ) { var element = this._currentElement; @@ -86,11 +87,13 @@ class ReactTestComponent { getPublicInstance(transaction: ReactTestReconcileTransaction): Object { var element = this._currentElement; - var options = this._hostContainerInfo; - if (options && options.createNodeMock) { - return options.createNodeMock(element); - } - return {}; + var hostContainerInfo = this._hostContainerInfo; + invariant( + hostContainerInfo, + 'hostContainerInfo should be populated before ' + + 'getPublicInstance is called.' + ); + return hostContainerInfo.createNodeMock(element); } toJSON(): ReactTestRendererJSON { From 7b8366fb94c304f0c4a9cf426fb3761b359060b0 Mon Sep 17 00:00:00 2001 From: Brandon Dail Date: Thu, 10 Nov 2016 13:58:14 -0600 Subject: [PATCH 3/5] Linting fixes --- src/renderers/testing/__tests__/ReactTestRenderer-test.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/renderers/testing/__tests__/ReactTestRenderer-test.js b/src/renderers/testing/__tests__/ReactTestRenderer-test.js index a9e47a6777ca7..d939fa86bc2c7 100644 --- a/src/renderers/testing/__tests__/ReactTestRenderer-test.js +++ b/src/renderers/testing/__tests__/ReactTestRenderer-test.js @@ -309,12 +309,12 @@ describe('ReactTestRenderer', () => { it('supports unmounting when using refs', () => { class Foo extends React.Component { render() { - return
; + return
; } } const inst = ReactTestRenderer.create( - , - { createNodeMock: () => 'foo' } + , + {createNodeMock: () => 'foo'} ); expect(() => inst.unmount()).not.toThrow(); }); @@ -334,7 +334,7 @@ describe('ReactTestRenderer', () => { } const inst = ReactTestRenderer.create( , - { createNodeMock } + {createNodeMock} ); inst.update(); // It's called with 'div' twice (mounting and unmounting) From a4b488f08f2b048c1d2ea22e4dda14241055b86c Mon Sep 17 00:00:00 2001 From: Brandon Dail Date: Thu, 10 Nov 2016 21:39:25 -0600 Subject: [PATCH 4/5] Remove transaction from ref lifecycle code path We don't need to pass the transaction around anymore since we store the test options on _hostContainerInfo instead --- scripts/fiber/tests-failing.txt | 3 --- scripts/fiber/tests-passing.txt | 1 + .../shared/stack/reconciler/ReactCompositeComponent.js | 4 ++-- src/renderers/shared/stack/reconciler/ReactOwner.js | 4 +--- src/renderers/shared/stack/reconciler/ReactReconciler.js | 8 ++------ src/renderers/shared/stack/reconciler/ReactRef.js | 9 +++------ src/renderers/testing/ReactTestMount.js | 4 ++-- src/renderers/testing/ReactTestRenderer.js | 2 +- 8 files changed, 12 insertions(+), 23 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index fb555d55ceaf6..a41b7230a1eca 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -589,9 +589,6 @@ src/renderers/shared/stack/reconciler/__tests__/ReactUpdates-test.js src/renderers/shared/stack/reconciler/__tests__/refs-test.js * Should increase refs with an increase in divs -src/renderers/testing/__tests__/ReactTestRenderer-test.js -* supports updates when using refs - src/shared/utils/__tests__/traverseAllChildren-test.js * should warn for using maps as children with owner info diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 8c2b937a3309f..df01f0f986237 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1207,6 +1207,7 @@ src/renderers/testing/__tests__/ReactTestRenderer-test.js * warns correctly for refs on SFCs * allows an optional createNodeMock function * supports unmounting when using refs +* supports updates when using refs * supports error boundaries src/shared/utils/__tests__/KeyEscapeUtils-test.js diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index 8e4e03072e960..d7ea65be32a3c 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -1205,10 +1205,10 @@ var ReactCompositeComponent = { * @final * @private */ - attachRef: function(ref, component, transaction) { + attachRef: function(ref, component) { var inst = this.getPublicInstance(); invariant(inst != null, 'Stateless function components cannot have refs.'); - var publicComponentInstance = component.getPublicInstance(transaction); + var publicComponentInstance = component.getPublicInstance(); if (__DEV__) { var componentName = component && component.getName ? component.getName() : 'a component'; diff --git a/src/renderers/shared/stack/reconciler/ReactOwner.js b/src/renderers/shared/stack/reconciler/ReactOwner.js index a804513d73583..c634dfb384432 100644 --- a/src/renderers/shared/stack/reconciler/ReactOwner.js +++ b/src/renderers/shared/stack/reconciler/ReactOwner.js @@ -15,7 +15,6 @@ var invariant = require('invariant'); import type { ReactInstance } from 'ReactInstanceType'; -import type { Transaction } from 'Transaction'; /** * @param {?object} object @@ -74,7 +73,6 @@ var ReactOwner = { component: ReactInstance, ref: string, owner: ReactInstance, - transaction: Transaction, ): void { invariant( isValidOwner(owner), @@ -83,7 +81,7 @@ var ReactOwner = { '`render` method, or you have multiple copies of React loaded ' + '(details: https://fb.me/react-refs-must-have-owner).' ); - owner.attachRef(ref, component, transaction); + owner.attachRef(ref, component); }, /** diff --git a/src/renderers/shared/stack/reconciler/ReactReconciler.js b/src/renderers/shared/stack/reconciler/ReactReconciler.js index f55bf339ee7aa..14a4191710d44 100644 --- a/src/renderers/shared/stack/reconciler/ReactReconciler.js +++ b/src/renderers/shared/stack/reconciler/ReactReconciler.js @@ -20,12 +20,8 @@ var warning = require('warning'); * Helper to call ReactRef.attachRefs with this composite component, split out * to avoid allocations in the transaction mount-ready queue. */ -function attachRefs(transaction) { - ReactRef.attachRefs( - this, - this._currentElement, - transaction, - ); +function attachRefs() { + ReactRef.attachRefs(this, this._currentElement); } var ReactReconciler = { diff --git a/src/renderers/shared/stack/reconciler/ReactRef.js b/src/renderers/shared/stack/reconciler/ReactRef.js index 12e04e81c02ca..a7350cc6a8a73 100644 --- a/src/renderers/shared/stack/reconciler/ReactRef.js +++ b/src/renderers/shared/stack/reconciler/ReactRef.js @@ -16,20 +16,18 @@ var ReactOwner = require('ReactOwner'); import type { ReactInstance } from 'ReactInstanceType'; import type { ReactElement } from 'ReactElementType'; -import type { Transaction } from 'Transaction'; var ReactRef = {}; -function attachRef(ref, component, owner, transaction) { +function attachRef(ref, component, owner) { if (typeof ref === 'function') { - ref(component.getPublicInstance(transaction)); + ref(component.getPublicInstance()); } else { // Legacy ref ReactOwner.addComponentAsRefTo( component, ref, owner, - transaction, ); } } @@ -46,14 +44,13 @@ function detachRef(ref, component, owner) { ReactRef.attachRefs = function( instance: ReactInstance, element: ReactElement | string | number | null | false, - transaction: Transaction, ): void { if (element === null || typeof element !== 'object') { return; } var ref = element.ref; if (ref != null) { - attachRef(ref, instance, element._owner, transaction); + attachRef(ref, instance, element._owner); } }; diff --git a/src/renderers/testing/ReactTestMount.js b/src/renderers/testing/ReactTestMount.js index 9bad563445cb0..695258c4d6480 100644 --- a/src/renderers/testing/ReactTestMount.js +++ b/src/renderers/testing/ReactTestMount.js @@ -49,9 +49,9 @@ TopLevelWrapper.isReactTopLevelWrapper = true; * Mounts this component and inserts it into the DOM. * * @param {ReactComponent} componentInstance The instance to mount. - * @param {number} rootID ID of the root node. - * @param {number} containerTag container element to mount into. * @param {ReactReconcileTransaction} transaction + * @param {Object} hostParent + * @param {Object} hostContainerInfo */ function mountComponentIntoNode( componentInstance, diff --git a/src/renderers/testing/ReactTestRenderer.js b/src/renderers/testing/ReactTestRenderer.js index 7b180f8d49690..840e045d7b391 100644 --- a/src/renderers/testing/ReactTestRenderer.js +++ b/src/renderers/testing/ReactTestRenderer.js @@ -85,7 +85,7 @@ class ReactTestComponent { this.updateChildren(nextElement.props.children, transaction, context); } - getPublicInstance(transaction: ReactTestReconcileTransaction): Object { + getPublicInstance(): Object { var element = this._currentElement; var hostContainerInfo = this._hostContainerInfo; invariant( From e755a3be526b41206eb492d562b62b8d277b3c3f Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 11 Nov 2016 14:33:57 +0000 Subject: [PATCH 5/5] Remove unused argument --- src/renderers/testing/ReactTestMount.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderers/testing/ReactTestMount.js b/src/renderers/testing/ReactTestMount.js index 695258c4d6480..65dd8c4d0a209 100644 --- a/src/renderers/testing/ReactTestMount.js +++ b/src/renderers/testing/ReactTestMount.js @@ -100,7 +100,7 @@ var ReactTestInstance = function(component) { ReactTestInstance.prototype.getInstance = function() { return this._component._renderedComponent.getPublicInstance(); }; -ReactTestInstance.prototype.update = function(nextElement, options) { +ReactTestInstance.prototype.update = function(nextElement) { invariant( this._component, "ReactTestRenderer: .update() can't be called after unmount."