From 3a7d6f53849c932dd226417da6642a735e00e6d2 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 22 Feb 2017 16:12:51 +0000 Subject: [PATCH 1/6] moved require(emptyObject) to inline calls to get around referential mismatch Due to how Jest and other mocking libraries work in userland, modules that are required might not be referentially equal at various stages of the application lifecycle. This isnt a perfect fix, but by having the require calls inline, it has a better liklihood of ensuring emptyObject is the same reference --- src/isomorphic/modern/class/ReactBaseClasses.js | 7 ++++--- src/renderers/shared/fiber/ReactChildFiber.js | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/isomorphic/modern/class/ReactBaseClasses.js b/src/isomorphic/modern/class/ReactBaseClasses.js index 506840289e2e7..22bc3efd85188 100644 --- a/src/isomorphic/modern/class/ReactBaseClasses.js +++ b/src/isomorphic/modern/class/ReactBaseClasses.js @@ -14,7 +14,6 @@ var ReactNoopUpdateQueue = require('ReactNoopUpdateQueue'); var canDefineProperty = require('canDefineProperty'); -var emptyObject = require('emptyObject'); var invariant = require('invariant'); var warning = require('warning'); @@ -24,7 +23,8 @@ var warning = require('warning'); function ReactComponent(props, context, updater) { this.props = props; this.context = context; - this.refs = emptyObject; + // emptyObject is inlined to help avoid module mocking referential mismatches + this.refs = require('emptyObject'); // We initialize the default updater but the real one gets injected by the // renderer. this.updater = updater || ReactNoopUpdateQueue; @@ -133,7 +133,8 @@ function ReactPureComponent(props, context, updater) { // Duplicated from ReactComponent. this.props = props; this.context = context; - this.refs = emptyObject; + // emptyObject is inlined to help avoid module mocking referential mismatches + this.refs = require('emptyObject'); // We initialize the default updater but the real one gets injected by the // renderer. this.updater = updater || ReactNoopUpdateQueue; diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 79f80c9e40887..822f1f9b935f2 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -32,7 +32,6 @@ var ReactFiber = require('ReactFiber'); var ReactTypeOfSideEffect = require('ReactTypeOfSideEffect'); var ReactTypeOfWork = require('ReactTypeOfWork'); -var emptyObject = require('emptyObject'); var getIteratorFn = require('getIteratorFn'); var invariant = require('invariant'); var ReactFeatureFlags = require('ReactFeatureFlags'); @@ -101,7 +100,8 @@ function coerceRef(current: Fiber | null, element: ReactElement) { return current.ref; } const ref = function(value) { - const refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs; + // emptyObject is inlined to help avoid module mocking referential mismatches + const refs = inst.refs === require('emptyObject') ? (inst.refs = {}) : inst.refs; if (value === null) { delete refs[stringRef]; } else { From b8cf75a5b43be21224e6b0291baaf30fa261b261 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 22 Feb 2017 16:16:21 +0000 Subject: [PATCH 2/6] updated tests-failing and test-passing --- scripts/fiber/tests-failing.txt | 3 --- scripts/fiber/tests-passing.txt | 1 + 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 36eaf9ae75cb1..51334e9a2555b 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -19,9 +19,6 @@ src/renderers/__tests__/ReactPerf-test.js * should not count time in a portal towards lifecycle method * should work when measurement starts during reconciliation -src/renderers/__tests__/refs-test.js -* Should increase refs with an increase in divs - src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * gives source code refs for unknown prop warning (ssr) * gives source code refs for unknown prop warning for exact elements (ssr) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 072fe0598897f..0bbe916814f87 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -841,6 +841,7 @@ src/renderers/__tests__/refs-destruction-test.js * should not error when destroying child with ref asynchronously src/renderers/__tests__/refs-test.js +* Should increase refs with an increase in divs * Allow refs to hop around children correctly * always has a value for this.refs * ref called correctly for stateless component when __DEV__ = false From 724ae9b35a287dc183894f60f2750fdb2815684f Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 23 Feb 2017 12:31:58 +0000 Subject: [PATCH 3/6] reverts previous changes to inlining requires and fixes test suite instead --- .../modern/class/ReactBaseClasses.js | 7 +- src/renderers/__tests__/refs-test.js | 131 +++++++++--------- src/renderers/shared/fiber/ReactChildFiber.js | 4 +- 3 files changed, 70 insertions(+), 72 deletions(-) diff --git a/src/isomorphic/modern/class/ReactBaseClasses.js b/src/isomorphic/modern/class/ReactBaseClasses.js index 22bc3efd85188..506840289e2e7 100644 --- a/src/isomorphic/modern/class/ReactBaseClasses.js +++ b/src/isomorphic/modern/class/ReactBaseClasses.js @@ -14,6 +14,7 @@ var ReactNoopUpdateQueue = require('ReactNoopUpdateQueue'); var canDefineProperty = require('canDefineProperty'); +var emptyObject = require('emptyObject'); var invariant = require('invariant'); var warning = require('warning'); @@ -23,8 +24,7 @@ var warning = require('warning'); function ReactComponent(props, context, updater) { this.props = props; this.context = context; - // emptyObject is inlined to help avoid module mocking referential mismatches - this.refs = require('emptyObject'); + this.refs = emptyObject; // We initialize the default updater but the real one gets injected by the // renderer. this.updater = updater || ReactNoopUpdateQueue; @@ -133,8 +133,7 @@ function ReactPureComponent(props, context, updater) { // Duplicated from ReactComponent. this.props = props; this.context = context; - // emptyObject is inlined to help avoid module mocking referential mismatches - this.refs = require('emptyObject'); + this.refs = emptyObject; // We initialize the default updater but the real one gets injected by the // renderer. this.updater = updater || ReactNoopUpdateQueue; diff --git a/src/renderers/__tests__/refs-test.js b/src/renderers/__tests__/refs-test.js index 8cf2c5aa7ab18..d6f132a194df0 100644 --- a/src/renderers/__tests__/refs-test.js +++ b/src/renderers/__tests__/refs-test.js @@ -16,78 +16,80 @@ var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); var ReactTestUtils = require('ReactTestUtils'); /** - * Counts clicks and has a renders an item for each click. Each item rendered - * has a ref of the form "clickLogN". + * Render a TestRefsComponent and ensure that the main refs are wired up. + * we also define the classes in this function to ensure that they are + * wired up properly in accordance to how Jest might auto mock modules */ -class ClickCounter extends React.Component { - state = {count: this.props.initialCount}; - - triggerReset = () => { - this.setState({count: this.props.initialCount}); - }; - - handleClick = () => { - this.setState({count: this.state.count + 1}); - }; - - render() { - var children = []; - var i; - for (i = 0; i < this.state.count; i++) { - children.push( -
- ); +function renderTestRefsComponent() { + /** + * Only purpose is to test that refs are tracked even when applied to a + * component that is injected down several layers. Ref systems are difficult to + * build in such a way that ownership is maintained in an airtight manner. + */ + class GeneralContainerComponent extends React.Component { + render() { + return
{this.props.children}
; } - return ( - - {children} - - ); } -} -/** - * Only purpose is to test that refs are tracked even when applied to a - * component that is injected down several layers. Ref systems are difficult to - * build in such a way that ownership is maintained in an airtight manner. - */ -class GeneralContainerComponent extends React.Component { - render() { - return
{this.props.children}
; + /** + * Counts clicks and has a renders an item for each click. Each item rendered + * has a ref of the form "clickLogN". + */ + class ClickCounter extends React.Component { + state = {count: this.props.initialCount}; + + triggerReset = () => { + this.setState({count: this.props.initialCount}); + }; + + handleClick = () => { + this.setState({count: this.state.count + 1}); + }; + + render() { + var children = []; + var i; + for (i = 0; i < this.state.count; i++) { + children.push( +
+ ); + } + return ( + + {children} + + ); + } } -} -/** - * Notice how refs ownership is maintained even when injecting a component - * into a different parent. - */ -class TestRefsComponent extends React.Component { - doReset = () => { - this.refs.myCounter.triggerReset(); - }; - - render() { - return ( -
-
- Reset Me By Clicking This. + /** + * Notice how refs ownership is maintained even when injecting a component + * into a different parent. + */ + class TestRefsComponent extends React.Component { + doReset = () => { + this.refs.myCounter.triggerReset(); + }; + + render() { + return ( +
+
+ Reset Me By Clicking This. +
+ + +
- - - -
- ); + ); + } } -} -/** - * Render a TestRefsComponent and ensure that the main refs are wired up. - */ -var renderTestRefsComponent = function() { var testRefsComponent = ReactTestUtils.renderIntoDocument(); expect(testRefsComponent instanceof TestRefsComponent).toBe(true); @@ -146,11 +148,8 @@ describe('reactiverefs', () => { expectClickLogsLengthToBe(testRefsComponent, 1); }); - }); - - /** * Tests that when a ref hops around children, we can track that correctly. */ diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 822f1f9b935f2..79f80c9e40887 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -32,6 +32,7 @@ var ReactFiber = require('ReactFiber'); var ReactTypeOfSideEffect = require('ReactTypeOfSideEffect'); var ReactTypeOfWork = require('ReactTypeOfWork'); +var emptyObject = require('emptyObject'); var getIteratorFn = require('getIteratorFn'); var invariant = require('invariant'); var ReactFeatureFlags = require('ReactFeatureFlags'); @@ -100,8 +101,7 @@ function coerceRef(current: Fiber | null, element: ReactElement) { return current.ref; } const ref = function(value) { - // emptyObject is inlined to help avoid module mocking referential mismatches - const refs = inst.refs === require('emptyObject') ? (inst.refs = {}) : inst.refs; + const refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs; if (value === null) { delete refs[stringRef]; } else { From a749083b853451eaa6ff8cf55a5187c52f0420c7 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 23 Feb 2017 12:42:21 +0000 Subject: [PATCH 4/6] fixed comment that was incorrect --- src/renderers/__tests__/refs-test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/renderers/__tests__/refs-test.js b/src/renderers/__tests__/refs-test.js index d6f132a194df0..17d6b5a63ad8d 100644 --- a/src/renderers/__tests__/refs-test.js +++ b/src/renderers/__tests__/refs-test.js @@ -18,7 +18,8 @@ var ReactTestUtils = require('ReactTestUtils'); /** * Render a TestRefsComponent and ensure that the main refs are wired up. * we also define the classes in this function to ensure that they are - * wired up properly in accordance to how Jest might auto mock modules + * wired up properly in accordance to how the test suite might resetModules() + * between test runs */ function renderTestRefsComponent() { /** From 25f4abc9d5f63ae640cb07d4a23d664e9b1880ab Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 23 Feb 2017 12:58:37 +0000 Subject: [PATCH 5/6] mountClassInstance() now re-assigns instance.refs to emptyObject --- src/renderers/__tests__/refs-test.js | 132 +++++++++--------- .../shared/fiber/ReactFiberClassComponent.js | 1 + 2 files changed, 67 insertions(+), 66 deletions(-) diff --git a/src/renderers/__tests__/refs-test.js b/src/renderers/__tests__/refs-test.js index 17d6b5a63ad8d..8cf2c5aa7ab18 100644 --- a/src/renderers/__tests__/refs-test.js +++ b/src/renderers/__tests__/refs-test.js @@ -16,81 +16,78 @@ var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); var ReactTestUtils = require('ReactTestUtils'); /** - * Render a TestRefsComponent and ensure that the main refs are wired up. - * we also define the classes in this function to ensure that they are - * wired up properly in accordance to how the test suite might resetModules() - * between test runs + * Counts clicks and has a renders an item for each click. Each item rendered + * has a ref of the form "clickLogN". */ -function renderTestRefsComponent() { - /** - * Only purpose is to test that refs are tracked even when applied to a - * component that is injected down several layers. Ref systems are difficult to - * build in such a way that ownership is maintained in an airtight manner. - */ - class GeneralContainerComponent extends React.Component { - render() { - return
{this.props.children}
; - } - } - - /** - * Counts clicks and has a renders an item for each click. Each item rendered - * has a ref of the form "clickLogN". - */ - class ClickCounter extends React.Component { - state = {count: this.props.initialCount}; - - triggerReset = () => { - this.setState({count: this.props.initialCount}); - }; - - handleClick = () => { - this.setState({count: this.state.count + 1}); - }; - - render() { - var children = []; - var i; - for (i = 0; i < this.state.count; i++) { - children.push( -
- ); - } - return ( - - {children} - +class ClickCounter extends React.Component { + state = {count: this.props.initialCount}; + + triggerReset = () => { + this.setState({count: this.props.initialCount}); + }; + + handleClick = () => { + this.setState({count: this.state.count + 1}); + }; + + render() { + var children = []; + var i; + for (i = 0; i < this.state.count; i++) { + children.push( +
); } + return ( + + {children} + + ); } +} - /** - * Notice how refs ownership is maintained even when injecting a component - * into a different parent. - */ - class TestRefsComponent extends React.Component { - doReset = () => { - this.refs.myCounter.triggerReset(); - }; +/** + * Only purpose is to test that refs are tracked even when applied to a + * component that is injected down several layers. Ref systems are difficult to + * build in such a way that ownership is maintained in an airtight manner. + */ +class GeneralContainerComponent extends React.Component { + render() { + return
{this.props.children}
; + } +} - render() { - return ( -
-
- Reset Me By Clicking This. -
- - - +/** + * Notice how refs ownership is maintained even when injecting a component + * into a different parent. + */ +class TestRefsComponent extends React.Component { + doReset = () => { + this.refs.myCounter.triggerReset(); + }; + + render() { + return ( +
+
+ Reset Me By Clicking This.
- ); - } + + + +
+ ); } +} +/** + * Render a TestRefsComponent and ensure that the main refs are wired up. + */ +var renderTestRefsComponent = function() { var testRefsComponent = ReactTestUtils.renderIntoDocument(); expect(testRefsComponent instanceof TestRefsComponent).toBe(true); @@ -149,8 +146,11 @@ describe('reactiverefs', () => { expectClickLogsLengthToBe(testRefsComponent, 1); }); + }); + + /** * Tests that when a ref hops around children, we can track that correctly. */ diff --git a/src/renderers/shared/fiber/ReactFiberClassComponent.js b/src/renderers/shared/fiber/ReactFiberClassComponent.js index 87f3426099e48..cd47bd488c38a 100644 --- a/src/renderers/shared/fiber/ReactFiberClassComponent.js +++ b/src/renderers/shared/fiber/ReactFiberClassComponent.js @@ -291,6 +291,7 @@ module.exports = function( instance.props = props; instance.state = state; + instance.refs = emptyObject; instance.context = getMaskedContext(workInProgress, unmaskedContext); if (typeof instance.componentWillMount === 'function') { From a99d77e0732fcfd0dc9e3afb090499e4a7076557 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 23 Feb 2017 14:00:02 +0000 Subject: [PATCH 6/6] added a regression test for factory components --- scripts/fiber/tests-passing.txt | 1 + src/renderers/__tests__/refs-test.js | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 0bbe916814f87..38170c348cb78 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -842,6 +842,7 @@ src/renderers/__tests__/refs-destruction-test.js src/renderers/__tests__/refs-test.js * Should increase refs with an increase in divs +* Should correctly get the ref * Allow refs to hop around children correctly * always has a value for this.refs * ref called correctly for stateless component when __DEV__ = false diff --git a/src/renderers/__tests__/refs-test.js b/src/renderers/__tests__/refs-test.js index 8cf2c5aa7ab18..10b5d54ac8f38 100644 --- a/src/renderers/__tests__/refs-test.js +++ b/src/renderers/__tests__/refs-test.js @@ -144,12 +144,23 @@ describe('reactiverefs', () => { // Now reset again ReactTestUtils.Simulate.click(testRefsComponent.refs.resetDiv); expectClickLogsLengthToBe(testRefsComponent, 1); - }); - }); +describe('factory components', () => { + it('Should correctly get the ref', () => { + function Comp() { + return { + render() { + return
; + }, + }; + } + const inst = ReactTestUtils.renderIntoDocument(); + expect(inst.refs.elemRef.tagName).toBe('DIV'); + }); +}); /** * Tests that when a ref hops around children, we can track that correctly.