From c4bd356d2d2f0e96115535f916ab7aa1d84ae7c9 Mon Sep 17 00:00:00 2001 From: Dustan Kasten Date: Wed, 11 Jan 2017 14:49:26 -0500 Subject: [PATCH 1/8] Fix ReactFiberReconciler annotation to include `PI` https://github.com/facebook/react/pull/8628#issuecomment-271970297 --- src/renderers/shared/fiber/ReactFiberReconciler.js | 2 +- src/renderers/testing/ReactTestRendererFiber.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index 94c843b4a1a4a..9e778d8911489 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -170,7 +170,7 @@ module.exports = function(config : HostConfig | I | TI | null) { + getPublicRootInstance(container : OpaqueNode) : (ReactComponent | PI | null) { const root : FiberRoot = (container.stateNode : any); const containerFiber = root.current; if (!containerFiber.child) { diff --git a/src/renderers/testing/ReactTestRendererFiber.js b/src/renderers/testing/ReactTestRendererFiber.js index e9db09a3ce3db..600d02664699c 100644 --- a/src/renderers/testing/ReactTestRendererFiber.js +++ b/src/renderers/testing/ReactTestRendererFiber.js @@ -191,7 +191,7 @@ var TestRenderer = ReactFiberReconciler({ useSyncScheduling: true, - getPublicInstance(inst) { + getPublicInstance(inst: Instance | TextInstance) : * { switch (inst.tag) { case 'INSTANCE': const createNodeMock = inst.rootContainerInstance.createNodeMock; From 2e942c78d137f8e469fc56de8c2a3a01097c29ed Mon Sep 17 00:00:00 2001 From: Dustan Kasten Date: Wed, 11 Jan 2017 15:22:08 -0500 Subject: [PATCH 2/8] Make getPublicInstance type safe --- src/renderers/shared/fiber/ReactFiberCommitWork.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 37bef63964c2b..9fbab9c835c8c 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -460,8 +460,14 @@ module.exports = function( } const ref = finishedWork.ref; if (ref) { - const instance = getPublicInstance(finishedWork.stateNode); - ref(instance); + const instance = finishedWork.stateNode; + switch (finishedWork.tag) { + case HostComponent: + ref(getPublicInstance(instance)); + break; + default: + ref(instance); + } } } From 662844ca75f47cbeac81a217668773b973c67920 Mon Sep 17 00:00:00 2001 From: Dustan Kasten Date: Fri, 13 Jan 2017 07:26:36 -0500 Subject: [PATCH 3/8] attempting to trigger the issue @sebmarkbage described https://github.com/facebook/react/pull/8751#discussion_r95669118 --- scripts/fiber/tests-passing.txt | 2 + .../shared/shared/__tests__/refs-test.js | 89 ++++++++++++++++++- 2 files changed, 90 insertions(+), 1 deletion(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 21c984271651c..52768306decaf 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1621,6 +1621,8 @@ src/renderers/shared/shared/__tests__/refs-test.js * coerces numbers to strings * attaches, detaches from fiber component with stack layer * attaches, detaches from stack component with fiber layer +* attaches and detaches root refs in stack +* attaches and detaches root refs in fiber src/renderers/shared/shared/event/__tests__/EventPluginHub-test.js * should prevent non-function listeners, at dispatch diff --git a/src/renderers/shared/shared/__tests__/refs-test.js b/src/renderers/shared/shared/__tests__/refs-test.js index 8cf2c5aa7ab18..b2fc2a8af2e4a 100644 --- a/src/renderers/shared/shared/__tests__/refs-test.js +++ b/src/renderers/shared/shared/__tests__/refs-test.js @@ -230,7 +230,6 @@ describe('ref swapping', () => { expect(refHopsAround.refs.divThreeRef).toEqual(thirdDiv); }); - it('always has a value for this.refs', () => { class Component extends React.Component { render() { @@ -392,3 +391,91 @@ describe('string refs between fiber and stack', () => { } }); }); + +describe('root level refs', () => { + it('attaches and detaches root refs in stack', () => { + assertForRenderer('ReactDOM'); + }); + + it('attaches and detaches root refs in fiber', () => { + assertForRenderer('ReactDOMFiber'); + }); + + const assertForRenderer = (which) => { + const Renderer = require(which); + spyOn(console, 'error'); + var inst = null; + + // host node + var ref = jest.fn(value => inst = value); + var container = document.createElement('div'); + var result = Renderer.render(
, container); + expect(ref).toHaveBeenCalledTimes(1); + expect(ref.mock.calls[0][0]).toBeInstanceOf(HTMLDivElement); + expect(result).toBe(ref.mock.calls[0][0]); + Renderer.unmountComponentAtNode(container); + expect(ref).toHaveBeenCalledTimes(2); + expect(ref.mock.calls[1][0]).toBe(null); + + // composite + class Comp extends React.Component { + method() { + return true; + } + render() { + return
Comp
; + } + } + + inst = null; + ref = jest.fn(value => inst = value); + result = Renderer.render(, container); + + expect(ref).toHaveBeenCalledTimes(1); + expect(inst).toBeInstanceOf(Comp); + expect(result).toBe(inst); + + // ensure we have the correct instance + expect(result.method()).toBe(true); + expect(inst.method()).toBe(true); + + Renderer.unmountComponentAtNode(container); + expect(ref).toHaveBeenCalledTimes(2); + expect(ref.mock.calls[1][0]).toBe(null); + + if (which === 'ReactDOMFiber') { + // fragment + inst = null; + ref = jest.fn(value => inst = value); + var divInst = null; + var ref2 = jest.fn(value => divInst = value); + result = Renderer.render([ + , + 5, +
Hello
, + ], container); + + // first call should be `Comp` + expect(ref).toHaveBeenCalledTimes(1); + expect(ref.mock.calls[0][0]).toBeInstanceOf(Comp); + expect(result).toBe(ref.mock.calls[0][0]); + + expect(ref2).toHaveBeenCalledTimes(1); + expect(divInst).toBeInstanceOf(HTMLDivElement); + expect(result).not.toBe(divInst); + Renderer.unmountComponentAtNode(container); + expect(ref).toHaveBeenCalledTimes(2); + expect(ref.mock.calls[1][0]).toBe(null); + expect(ref2).toHaveBeenCalledTimes(2); + expect(ref2.mock.calls[1][0]).toBe(null); + + // null + result = Renderer.render(null, container); + expect(result).toBe(null); + + // primitives + // result = Renderer.render(5, container); + // expect(result).toBe('5'); + } + }; +}); From fc91ebbff90016809ac81f67821caab5d794e119 Mon Sep 17 00:00:00 2001 From: Dustan Kasten Date: Mon, 16 Jan 2017 08:15:24 -0500 Subject: [PATCH 4/8] Unify renderer-specific getPublicInstance with getRootInstance --- scripts/fiber/tests-failing.txt | 12 ++++++++++++ scripts/fiber/tests-passing.txt | 9 +-------- src/renderers/shared/fiber/ReactFiberReconciler.js | 6 +++++- .../testing/__tests__/ReactTestRenderer-test.js | 11 +++++++++++ 4 files changed, 29 insertions(+), 9 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index fdde0f21ec20c..d54cc67c08d6a 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -3,9 +3,16 @@ src/addons/__tests__/ReactFragment-test.js * should throw if a plain object even if it is in an owner * should throw if a plain object looks like an old element +src/addons/transitions/__tests__/ReactCSSTransitionGroup-test.js +* should clean-up silently after the timeout elapses + src/isomorphic/classic/__tests__/ReactContextValidator-test.js * should pass previous context to lifecycles +src/isomorphic/modern/class/__tests__/ReactClassEquivalence-test.js +* tests the same thing for es6 classes and CoffeeScript +* tests the same thing for es6 classes and TypeScript + src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production @@ -37,6 +44,11 @@ src/renderers/dom/shared/__tests__/ReactRenderDocument-test.js * should throw on full document render w/ no markup * supports findDOMNode on full-page components +src/renderers/dom/shared/__tests__/ReactServerRendering-test.js +* warns with a no-op when an async setState is triggered +* warns with a no-op when an async replaceState is triggered +* warns with a no-op when an async forceUpdate is triggered + src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js * should control a value in reentrant events diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 52768306decaf..479284461185b 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -82,7 +82,6 @@ src/addons/__tests__/update-test.js src/addons/transitions/__tests__/ReactCSSTransitionGroup-test.js * should warn if timeouts aren't specified * should not warn if timeouts is zero -* should clean-up silently after the timeout elapses * should keep both sets of DOM nodes around * should switch transitionLeave from false to true * should work with no children @@ -386,10 +385,6 @@ src/isomorphic/classic/types/__tests__/ReactPropTypesProduction-test.js * should be a no-op * should not have been called -src/isomorphic/modern/class/__tests__/ReactClassEquivalence-test.js -* tests the same thing for es6 classes and CoffeeScript -* tests the same thing for es6 classes and TypeScript - src/isomorphic/modern/class/__tests__/ReactCoffeeScriptClass-test.coffee * preserves the name of the class for use in error messages * throws if no render function is defined @@ -785,9 +780,6 @@ src/renderers/dom/shared/__tests__/ReactServerRendering-test.js * should throw with silly args * allows setState in componentWillMount without using DOM * renders components with different batching strategies -* warns with a no-op when an async setState is triggered -* warns with a no-op when an async replaceState is triggered -* warns with a no-op when an async forceUpdate is triggered * should warn when children are mutated during render src/renderers/dom/shared/__tests__/escapeTextContentForBrowser-test.js @@ -1708,6 +1700,7 @@ src/renderers/testing/__tests__/ReactTestRenderer-test.js * supports updates when using refs * supports error boundaries * can update text nodes +* root instance and createNodeMock ref return the same value * can update text nodes when rendered as root * can render and update root fragments diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index 9e778d8911489..e278f9746a962 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -104,6 +104,10 @@ getContextForSubtree._injectFiber(function(fiber : Fiber) { module.exports = function(config : HostConfig) : Reconciler { + var { + getPublicInstance, + } = config; + var { scheduleUpdate, getPriorityContext, @@ -176,7 +180,7 @@ module.exports = function(config : HostConfig { }); }); + it('root instance and createNodeMock ref return the same value', () => { + var createNodeMock = ref => ({node: ref}); + var refInst = null; + var renderer = ReactTestRenderer.create( +
refInst = ref} />, + {createNodeMock} + ); + var root = renderer.getInstance(); + expect(root).toEqual(refInst); + }); + if (ReactDOMFeatureFlags.useFiber) { it('can update text nodes when rendered as root', () => { var renderer = ReactTestRenderer.create(['Hello', 'world']); From 5156c65e4059a7f7148bbd929f75ea264c66cd13 Mon Sep 17 00:00:00 2001 From: Dustan Kasten Date: Sat, 21 Jan 2017 13:36:09 -0500 Subject: [PATCH 5/8] Switch on fiber type HostComponent for getPublicRootInstance --- src/renderers/shared/fiber/ReactFiberReconciler.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index e278f9746a962..87b2ad237cf7b 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -28,6 +28,9 @@ var { } = require('ReactFiberContext'); var { createFiberRoot } = require('ReactFiberRoot'); var ReactFiberScheduler = require('ReactFiberScheduler'); +var { + HostComponent, +} = require('ReactTypeOfWork'); if (__DEV__) { var ReactFiberInstrumentation = require('ReactFiberInstrumentation'); @@ -180,7 +183,12 @@ module.exports = function(config : HostConfig Date: Tue, 30 May 2017 16:23:44 +0100 Subject: [PATCH 6/8] Fix test that was too dynamic (and failing) --- scripts/fiber/tests-failing.txt | 3 -- scripts/fiber/tests-passing.txt | 2 +- src/renderers/__tests__/refs-test.js | 41 +++++++++++++--------------- 3 files changed, 20 insertions(+), 26 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index d49975e7ccaea..805b7a5513a6f 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -1,6 +1,3 @@ -src/renderers/__tests__/refs-test.js -* attaches and detaches root refs in fiber - src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js * should not blow away user-entered text on successful reconnect to an uncontrolled checkbox * should not blow away user-entered text on successful reconnect to a controlled checkbox diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 9c2de64af3cf6..111ea1692e200 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -610,7 +610,7 @@ src/renderers/__tests__/refs-test.js * coerces numbers to strings * attaches, detaches from fiber component with stack layer * attaches, detaches from stack component with fiber layer -* attaches and detaches root refs in stack +* attaches and detaches root refs src/renderers/art/__tests__/ReactART-test.js * should have the correct lifecycle state diff --git a/src/renderers/__tests__/refs-test.js b/src/renderers/__tests__/refs-test.js index 2a281dce4f494..46bb991edfca2 100644 --- a/src/renderers/__tests__/refs-test.js +++ b/src/renderers/__tests__/refs-test.js @@ -411,27 +411,23 @@ describe('string refs between fiber and stack', () => { }); describe('root level refs', () => { - it('attaches and detaches root refs in stack', () => { - assertForRenderer('ReactDOM'); - }); - - it('attaches and detaches root refs in fiber', () => { - assertForRenderer('ReactDOMFiber'); + beforeEach(() => { + var ReactFeatureFlags = require('ReactFeatureFlags'); + ReactFeatureFlags.disableNewFiberFeatures = false; }); - const assertForRenderer = (which) => { - const Renderer = require(which); - spyOn(console, 'error'); + it('attaches and detaches root refs', () => { + var ReactDOM = require('react-dom'); var inst = null; // host node var ref = jest.fn(value => inst = value); var container = document.createElement('div'); - var result = Renderer.render(
, container); + var result = ReactDOM.render(
, container); expect(ref).toHaveBeenCalledTimes(1); expect(ref.mock.calls[0][0]).toBeInstanceOf(HTMLDivElement); expect(result).toBe(ref.mock.calls[0][0]); - Renderer.unmountComponentAtNode(container); + ReactDOM.unmountComponentAtNode(container); expect(ref).toHaveBeenCalledTimes(2); expect(ref.mock.calls[1][0]).toBe(null); @@ -447,7 +443,7 @@ describe('root level refs', () => { inst = null; ref = jest.fn(value => inst = value); - result = Renderer.render(, container); + result = ReactDOM.render(, container); expect(ref).toHaveBeenCalledTimes(1); expect(inst).toBeInstanceOf(Comp); @@ -457,20 +453,20 @@ describe('root level refs', () => { expect(result.method()).toBe(true); expect(inst.method()).toBe(true); - Renderer.unmountComponentAtNode(container); + ReactDOM.unmountComponentAtNode(container); expect(ref).toHaveBeenCalledTimes(2); expect(ref.mock.calls[1][0]).toBe(null); - if (which === 'ReactDOMFiber') { + if (ReactDOMFeatureFlags.useFiber) { // fragment inst = null; ref = jest.fn(value => inst = value); var divInst = null; var ref2 = jest.fn(value => divInst = value); - result = Renderer.render([ - , + result = ReactDOM.render([ + , 5, -
Hello
, +
Hello
, ], container); // first call should be `Comp` @@ -481,19 +477,20 @@ describe('root level refs', () => { expect(ref2).toHaveBeenCalledTimes(1); expect(divInst).toBeInstanceOf(HTMLDivElement); expect(result).not.toBe(divInst); - Renderer.unmountComponentAtNode(container); + + ReactDOM.unmountComponentAtNode(container); expect(ref).toHaveBeenCalledTimes(2); expect(ref.mock.calls[1][0]).toBe(null); expect(ref2).toHaveBeenCalledTimes(2); expect(ref2.mock.calls[1][0]).toBe(null); // null - result = Renderer.render(null, container); + result = ReactDOM.render(null, container); expect(result).toBe(null); // primitives - // result = Renderer.render(5, container); - // expect(result).toBe('5'); + result = ReactDOM.render(5, container); + expect(result).toBeInstanceOf(Text); } - }; + }); }); From 303eeffbf05f898f64eeacbc71a1c5cc39d6415c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 30 May 2017 16:32:25 +0100 Subject: [PATCH 7/8] Use PI in reconciler public API --- src/renderers/shared/fiber/ReactFiberReconciler.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index cff5e917794ce..3ecc06c8294e7 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -276,7 +276,7 @@ module.exports = function( getPublicRootInstance( container: OpaqueRoot, - ): ReactComponent | I | TI | PI | null { + ): ReactComponent | PI | null { const containerFiber = container.current; if (!containerFiber.child) { return null; @@ -289,7 +289,7 @@ module.exports = function( } }, - findHostInstance(fiber: Fiber): I | TI | null { + findHostInstance(fiber: Fiber): PI | null { const hostFiber = findCurrentHostFiber(fiber); if (hostFiber === null) { return null; From 7018be875a7832aea9d65af846c01c11321baa04 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 30 May 2017 16:35:24 +0100 Subject: [PATCH 8/8] Prettier --- src/renderers/__tests__/refs-test.js | 19 +++++++++---------- .../shared/fiber/ReactFiberReconciler.js | 8 ++------ .../testing/ReactTestRendererFiber.js | 2 +- .../__tests__/ReactTestRenderer-test.js | 4 ++-- 4 files changed, 14 insertions(+), 19 deletions(-) diff --git a/src/renderers/__tests__/refs-test.js b/src/renderers/__tests__/refs-test.js index 46bb991edfca2..05727c764d2d1 100644 --- a/src/renderers/__tests__/refs-test.js +++ b/src/renderers/__tests__/refs-test.js @@ -421,7 +421,7 @@ describe('root level refs', () => { var inst = null; // host node - var ref = jest.fn(value => inst = value); + var ref = jest.fn(value => (inst = value)); var container = document.createElement('div'); var result = ReactDOM.render(
, container); expect(ref).toHaveBeenCalledTimes(1); @@ -442,8 +442,8 @@ describe('root level refs', () => { } inst = null; - ref = jest.fn(value => inst = value); - result = ReactDOM.render(, container); + ref = jest.fn(value => (inst = value)); + result = ReactDOM.render(, container); expect(ref).toHaveBeenCalledTimes(1); expect(inst).toBeInstanceOf(Comp); @@ -460,14 +460,13 @@ describe('root level refs', () => { if (ReactDOMFeatureFlags.useFiber) { // fragment inst = null; - ref = jest.fn(value => inst = value); + ref = jest.fn(value => (inst = value)); var divInst = null; - var ref2 = jest.fn(value => divInst = value); - result = ReactDOM.render([ - , - 5, -
Hello
, - ], container); + var ref2 = jest.fn(value => (divInst = value)); + result = ReactDOM.render( + [, 5,
Hello
], + container, + ); // first call should be `Comp` expect(ref).toHaveBeenCalledTimes(1); diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index 3ecc06c8294e7..a7e41318647cd 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -28,9 +28,7 @@ var { } = require('ReactFiberContext'); var {createFiberRoot} = require('ReactFiberRoot'); var ReactFiberScheduler = require('ReactFiberScheduler'); -var { - HostComponent, -} = require('ReactTypeOfWork'); +var {HostComponent} = require('ReactTypeOfWork'); if (__DEV__) { var warning = require('fbjs/lib/warning'); @@ -170,9 +168,7 @@ getContextForSubtree._injectFiber(function(fiber: Fiber) { module.exports = function( config: HostConfig, ): Reconciler { - var { - getPublicInstance, - } = config; + var {getPublicInstance} = config; var { scheduleUpdate, diff --git a/src/renderers/testing/ReactTestRendererFiber.js b/src/renderers/testing/ReactTestRendererFiber.js index 66b31162b75f7..0fa7daced485c 100644 --- a/src/renderers/testing/ReactTestRendererFiber.js +++ b/src/renderers/testing/ReactTestRendererFiber.js @@ -219,7 +219,7 @@ var TestRenderer = ReactFiberReconciler({ useSyncScheduling: true, - getPublicInstance(inst: Instance | TextInstance) : * { + getPublicInstance(inst: Instance | TextInstance): * { switch (inst.tag) { case 'INSTANCE': const createNodeMock = inst.rootContainerInstance.createNodeMock; diff --git a/src/renderers/testing/__tests__/ReactTestRenderer-test.js b/src/renderers/testing/__tests__/ReactTestRenderer-test.js index 1c1c38bc6440a..81b0ea4c2bd25 100644 --- a/src/renderers/testing/__tests__/ReactTestRenderer-test.js +++ b/src/renderers/testing/__tests__/ReactTestRenderer-test.js @@ -572,8 +572,8 @@ describe('ReactTestRenderer', () => { var createNodeMock = ref => ({node: ref}); var refInst = null; var renderer = ReactTestRenderer.create( -
refInst = ref} />, - {createNodeMock} +
(refInst = ref)} />, + {createNodeMock}, ); var root = renderer.getInstance(); expect(root).toEqual(refInst);