From 4fd1802ddfaed1026e79af5e9ea355fdebc41399 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Thu, 1 Dec 2016 20:34:03 -0800 Subject: [PATCH 1/2] Additional findDOMNode tests This is failing in Fiber without the fix. Because we have no deletions to rely on in this case and the placement effects have already happened. --- scripts/fiber/tests-failing.txt | 6 +++++ .../dom/fiber/__tests__/ReactDOMFiber-test.js | 24 +++++++++++++++++ .../dom/shared/__tests__/findDOMNode-test.js | 26 +++++++++++++++++++ 3 files changed, 56 insertions(+) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 0aba62db6feb..f5338b459831 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -27,6 +27,9 @@ src/renderers/art/__tests__/ReactART-test.js src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production +src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +* findDOMNode should find dom element after expanding a fragment + src/renderers/dom/shared/__tests__/ReactDOM-test.js * throws in render() if the mount callback is not a function * throws in render() if the update callback is not a function @@ -63,6 +66,9 @@ 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__/findDOMNode-test.js +* findDOMNode should find dom element after an update from null + src/renderers/shared/__tests__/ReactPerf-test.js * should count no-op update as waste * should count no-op update in child as waste diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index 13f6a41499b3..4fb7cd213714 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js @@ -459,5 +459,29 @@ describe('ReactDOMFiber', () => { expect(portalContainer.innerHTML).toBe('
changed-changed
'); expect(container.innerHTML).toBe(''); }); + + it('findDOMNode should find dom element after expanding a fragment', () => { + class MyNode extends React.Component { + render() { + return ( + !this.props.flag ? + [
] : + [,
] + ); + } + } + + var container = document.createElement('div'); + + var myNodeA = ReactDOM.render(, container); + var a = ReactDOM.findDOMNode(myNodeA); + expect(a.tagName).toBe('DIV'); + + var myNodeB = ReactDOM.render(, container); + expect(myNodeA === myNodeB).toBe(true); + + var b = ReactDOM.findDOMNode(myNodeB); + expect(b.tagName).toBe('SPAN'); + }); } }); diff --git a/src/renderers/dom/shared/__tests__/findDOMNode-test.js b/src/renderers/dom/shared/__tests__/findDOMNode-test.js index ea1e72e58c14..e7f7aba6d3a1 100644 --- a/src/renderers/dom/shared/__tests__/findDOMNode-test.js +++ b/src/renderers/dom/shared/__tests__/findDOMNode-test.js @@ -34,6 +34,32 @@ describe('findDOMNode', () => { expect(mySameDiv).toBe(myDiv); }); + it('findDOMNode should find dom element after an update from null', () => { + function Bar({ flag }) { + if (flag) { + return A; + } + return null; + } + class MyNode extends React.Component { + render() { + return ; + } + } + + var container = document.createElement('div'); + + var myNodeA = ReactDOM.render(, container); + var a = ReactDOM.findDOMNode(myNodeA); + expect(a).toBe(null); + + var myNodeB = ReactDOM.render(, container); + expect(myNodeA === myNodeB).toBe(true); + + var b = ReactDOM.findDOMNode(myNodeB); + expect(b.tagName).toBe('SPAN'); + }); + it('findDOMNode should reject random objects', () => { expect(function() { ReactDOM.findDOMNode({foo: 'bar'}); From a434f9e7afc2200787ab8cfa96e7641d98d3ce69 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Thu, 1 Dec 2016 21:21:05 -0800 Subject: [PATCH 2/2] Fix findDOMNode and findAllInRenderedFiberTreeInternal This strategy finds the current fiber. It traverses back up to the root if the two trees are completely separate and determines which one is current. If the two trees converge anywhere along the way, we assume that is the current tree. We find the current child by searching the converged child set. This could fail if there's any way for both return pointers to point backwards to the work in progress. I don't think there is but I could be wrong. This may also fail on coroutines where we have reparenting situations. --- scripts/fiber/tests-failing.txt | 9 -- scripts/fiber/tests-passing.txt | 3 + .../shared/fiber/ReactFiberScheduler.js | 3 + .../shared/fiber/ReactFiberTreeReflection.js | 118 +++++++++++++----- src/test/ReactTestUtils.js | 78 +++++------- 5 files changed, 126 insertions(+), 85 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index f5338b459831..16dd1717af37 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -27,9 +27,6 @@ src/renderers/art/__tests__/ReactART-test.js src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production -src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js -* findDOMNode should find dom element after expanding a fragment - src/renderers/dom/shared/__tests__/ReactDOM-test.js * throws in render() if the mount callback is not a function * throws in render() if the update callback is not a function @@ -66,9 +63,6 @@ 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__/findDOMNode-test.js -* findDOMNode should find dom element after an update from null - src/renderers/shared/__tests__/ReactPerf-test.js * should count no-op update as waste * should count no-op update in child as waste @@ -125,6 +119,3 @@ src/renderers/shared/shared/__tests__/ReactUpdates-test.js src/renderers/shared/shared/__tests__/refs-test.js * Should increase refs with an increase in divs - -src/test/__tests__/ReactTestUtils-test.js -* traverses children in the correct order diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 4459f673833f..a1401beb4089 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -507,6 +507,7 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * should pass portal context when rendering subtree elsewhere * should update portal context if it changes due to setState * should update portal context if it changes due to re-render +* findDOMNode should find dom element after expanding a fragment src/renderers/dom/shared/__tests__/CSSProperty-test.js * should generate browser prefixes for its `isUnitlessNumber` @@ -713,6 +714,7 @@ src/renderers/dom/shared/__tests__/escapeTextContentForBrowser-test.js src/renderers/dom/shared/__tests__/findDOMNode-test.js * findDOMNode should return null if passed null * findDOMNode should find dom element +* findDOMNode should find dom element after an update from null * findDOMNode should reject random objects * findDOMNode should reject unmounted objects with render func * findDOMNode should not throw an error when called within a component that is not mounted @@ -1589,6 +1591,7 @@ src/test/__tests__/ReactTestUtils-test.js * can scryRenderedDOMComponentsWithClass with TextComponent * can scryRenderedDOMComponentsWithClass with className contains \n * can scryRenderedDOMComponentsWithClass with multiple classes +* traverses children in the correct order * should support injected wrapper components as DOM components * should change the value of an input field * should change the value of an input field in a component diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 4154a365255f..2798912747c0 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -183,6 +183,9 @@ module.exports = function(config : HostConfig) { commitPlacement(effectfulFiber); // Clear the "placement" from effect tag so that we know that this is inserted, before // any life-cycles like componentDidMount gets called. + // TODO: findDOMNode doesn't rely on this any more but isMounted + // does and isMounted is deprecated anyway so we should be able + // to kill this. effectfulFiber.effectTag &= ~Placement; break; } diff --git a/src/renderers/shared/fiber/ReactFiberTreeReflection.js b/src/renderers/shared/fiber/ReactFiberTreeReflection.js index 21389c0c422d..30694f28cc82 100644 --- a/src/renderers/shared/fiber/ReactFiberTreeReflection.js +++ b/src/renderers/shared/fiber/ReactFiberTreeReflection.js @@ -73,60 +73,116 @@ exports.isMounted = function(component : ReactComponent) : boolea return isFiberMountedImpl(fiber) === MOUNTED; }; -exports.findCurrentHostFiber = function(parent : Fiber) : Fiber | null { - // First check if this node itself is mounted. - const state = isFiberMountedImpl(parent, true); - if (state === UNMOUNTED) { +function assertIsMounted(fiber) { + invariant( + isFiberMountedImpl(fiber) === MOUNTED, + 'Unable to find node on an unmounted component.' + ); +} + +function findCurrentFiberUsingSlowPath(fiber : Fiber) : Fiber | null { + let alternate = fiber.alternate; + if (!alternate) { + // If there is no alternate, then we only need to check if it is mounted. + const state = isFiberMountedImpl(fiber); invariant( - false, + state !== UNMOUNTED, 'Unable to find node on an unmounted component.' ); - } else if (state === MOUNTING) { - return null; + if (state === MOUNTING) { + return null; + } + return fiber; } + // If we have two possible branches, we'll walk backwards up to the root + // to see what path the root points to. On the way we may hit one of the + // special cases and we'll deal with them. + let a = fiber; + let b = alternate; + while (true) { + let parentA = a.return; + let parentB = b.return; + if (!parentA || !parentB) { + // We're at the root. + break; + } + if (parentA.child === parentB.child) { + // If both parents are the same, then that is the current parent. If + // they're different but point to the same child, then it doesn't matter. + // Regardless, whatever child they point to is the current child. + // So we can now determine which child is current by scanning the child + // list for either A or B. + let child = parentA.child; + while (child) { + if (child === a) { + // We've determined that A is the current branch. + assertIsMounted(parentA); + return fiber; + } + if (child === b) { + // We've determined that B is the current branch. + assertIsMounted(parentA); + return alternate; + } + child = child.sibling; + } + // We should never have an alternate for any mounting node. So the only + // way this could possibly happen is if this was unmounted, if at all. + invariant( + false, + 'Unable to find node on an unmounted component.' + ); + } + a = parentA; + b = parentB; + invariant( + a.alternate === b, + 'Return fibers should always be each others\' alternates.' + ); + } + // If the root is not a host container, we're in a disconnected tree. I.e. + // unmounted. + invariant( + a.tag === HostRoot, + 'Unable to find node on an unmounted component.' + ); + if (a.stateNode.current === a) { + // We've determined that A is the current branch. + return fiber; + } + // Otherwise B has to be current branch. + return alternate; +} +exports.findCurrentFiberUsingSlowPath = findCurrentFiberUsingSlowPath; - let didTryOtherTree = false; - - // If the component doesn't have a child we first check the alternate to see - // if it has any and if so, if those were just recently inserted. - if (!parent.child && parent.alternate) { - parent = parent.alternate; +exports.findCurrentHostFiber = function(parent : Fiber) : Fiber | null { + const currentParent = findCurrentFiberUsingSlowPath(parent); + if (!currentParent) { + return null; } // Next we'll drill down this component to find the first HostComponent/Text. - let node : Fiber = parent; + let node : Fiber = currentParent; while (true) { - if ((node.effectTag & Placement) !== NoEffect || !node.return) { - // If any node along the way was deleted, or is an insertion, that means - // that we're actually in a work in progress to update this component with - // a different component. We need to look in the "current" fiber instead. - if (!parent.alternate) { - return null; - } - if (didTryOtherTree) { - // Safety, to avoid an infinite loop if something goes wrong. - throw new Error('This should never hit this infinite loop.'); - } - didTryOtherTree = true; - node = parent = parent.alternate; - continue; - } if (node.tag === HostComponent || node.tag === HostText) { return node; } else if (node.child) { + // TODO: If we hit a Portal, we're supposed to skip it. // TODO: Coroutines need to visit the stateNode. + node.child.return = node; node = node.child; continue; } - if (node === parent) { + if (node === currentParent) { return null; } while (!node.sibling) { - if (!node.return || node.return === parent) { + if (!node.return || node.return === currentParent) { return null; } node = node.return; } + node.sibling.return = node.return; node = node.sibling; } // Flow needs the return null here, but ESLint complains about it. diff --git a/src/test/ReactTestUtils.js b/src/test/ReactTestUtils.js index 50a50b95daea..1a393d426b24 100644 --- a/src/test/ReactTestUtils.js +++ b/src/test/ReactTestUtils.js @@ -20,9 +20,9 @@ var ReactControlledComponent = require('ReactControlledComponent'); var ReactDOM = require('ReactDOM'); var ReactDOMComponentTree = require('ReactDOMComponentTree'); var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter'); +var ReactFiberTreeReflection = require('ReactFiberTreeReflection'); var ReactInstanceMap = require('ReactInstanceMap'); var ReactTypeOfWork = require('ReactTypeOfWork'); -var ReactTypeOfSideEffect = require('ReactTypeOfSideEffect'); var ReactGenericBatching = require('ReactGenericBatching'); var SyntheticEvent = require('SyntheticEvent'); var ReactShallowRenderer = require('ReactShallowRenderer'); @@ -37,10 +37,6 @@ var { HostComponent, HostText, } = ReactTypeOfWork; -var { - NoEffect, - Placement, -} = ReactTypeOfSideEffect; function Event(suffix) {} @@ -84,34 +80,40 @@ function findAllInRenderedFiberTreeInternal(fiber, test) { if (!fiber) { return []; } - if ( - fiber.tag !== ClassComponent && - fiber.tag !== FunctionalComponent && - fiber.tag !== HostComponent && - fiber.tag !== HostText - ) { + var currentParent = ReactFiberTreeReflection.findCurrentFiberUsingSlowPath( + fiber + ); + if (!currentParent) { return []; } - if ((fiber.effectTag & Placement) !== NoEffect || !fiber.return) { - // If any node along the way was deleted, or is an insertion, that means - // that we're actually in a work in progress to update this component with - // a different component. We need to look in the "current" fiber instead. - return null; - } - var publicInst = fiber.stateNode; - var ret = publicInst && test(publicInst) ? [publicInst] : []; - var child = fiber.child; - while (child) { - ret = ret.concat( - findAllInRenderedFiberTreeInternal( - child, - test - ) - ); - child = child.sibling; + let node = currentParent; + let ret = []; + while (true) { + if (node.tag === HostComponent || node.tag === HostText || + node.tag === ClassComponent || node.tag === FunctionalComponent) { + var publicInst = node.stateNode; + if (test(publicInst)) { + ret.push(publicInst); + } + } + if (node.child) { + // TODO: Coroutines need to visit the stateNode. + node.child.return = node; + node = node.child; + continue; + } + if (node === currentParent) { + return ret; + } + while (!node.sibling) { + if (!node.return || node.return === currentParent) { + return ret; + } + node = node.return; + } + node.sibling.return = node.return; + node = node.sibling; } - // TODO: visit stateNode for coroutines - return ret; } /** @@ -222,21 +224,7 @@ var ReactTestUtils = { ); var internalInstance = ReactInstanceMap.get(inst); if (internalInstance && typeof internalInstance.tag === 'number') { - var fiber = internalInstance; - if (!fiber.child && fiber.alternate) { - // When we don't have children, we try the alternate first to see if it - // has any current children first. - fiber = fiber.alternate; - } - var results = findAllInRenderedFiberTreeInternal(fiber, test); - if (results === null) { - // Null is a sentinel that indicates that this was the wrong fiber. - results = findAllInRenderedFiberTreeInternal(fiber.alternate, test); - if (results === null) { - throw new Error('This should never happen.'); - } - } - return results; + return findAllInRenderedFiberTreeInternal(internalInstance, test); } else { return findAllInRenderedStackTreeInternal(internalInstance, test); }