@samitouri / QOS-React-2 / commits / 045e5bce35

Add check for string and null 'rootElement' in ReactDOMFiber (#9869)

* Add check for string and null 'rootElement' in ReactDOMFiber **what is the change?:** Before we call 'rootElement.getAttribute' we check that the method is defined. **why make this change?:** There is an internal use case I found where 'rootElement' is a string and null at different points as the page is rendered. It looks like this method was added as part of support for re-hydration of server-side rendered content. I can't imagine we would want to reuse content if the rootnode is a string or null. Not sure if we want an earlier check that it's an element before this point. **test plan:** `yarn test` and I manually tested this fix in the internal case where it was breaking * Add test and improve check for non-element rootElement **what is the change?:** We use the nodeType to check that we have the correct type of rootElement, and we added a unit test. **why make this change?:** Improve this solution to the problem. **test plan:** `yarn test` * run ./scripts/fiber/record-tests

Flarnie Marchan committed Jun 7, 2017 at 07:51 UTC 045e5bce3507d86090dfc70e492e9f61213e06bc

3 files changed +11 -1

scripts/fiber/tests-passing.txt

@@ -1314,6 +1314,7 @@ src/renderers/dom/shared/__tests__/ReactRenderDocument-test.js

1314 * should not be able to unmount component from document node

1315 * should not be able to switch root constructors

1316 * should be able to mount into document

1317 +* renders over an existing text child without throwing

1318 * should give helpful errors on state desync

1319 * should throw on full document render w/ no markup

1320 * supports findDOMNode on full-page components

src/renderers/dom/fiber/ReactDOMFiber.js

+3 -1

@@ -116,7 +116,9 @@ function getReactRootElementInContainer(container: any) {

116

117 function shouldReuseContent(container) {

118 const rootElement = getReactRootElementInContainer(container);

119 - return !!(rootElement && rootElement.getAttribute(ID_ATTRIBUTE_NAME));

119 + return !!(rootElement &&

120 + rootElement.nodeType === ELEMENT_NODE &&

121 + rootElement.getAttribute(ID_ATTRIBUTE_NAME));

122 }

123

124 function shouldAutoFocusHostComponent(type: string, props: Props): boolean {

src/renderers/dom/shared/__tests__/ReactRenderDocument-test.js

@@ -178,6 +178,13 @@ describe('rendering React components at document', () => {

178 expect(testDocument.body.innerHTML).toBe('Hello world');

179 });

180

181 + it('renders over an existing text child without throwing', () => {

182 + const container = document.createElement('div');

183 + container.textContent = 'potato';

184 + ReactDOM.render(<div>parsnip</div>, container);

185 + expect(container.textContent).toBe('parsnip');

186 + });

187 +

188 it('should give helpful errors on state desync', () => {

189 class Component extends React.Component {

190 render() {