From 1d5cd4d3980d8a13f514e00d7cf5d3209053b658 Mon Sep 17 00:00:00 2001 From: marc hurabielle Date: Thu, 27 Sep 2018 22:24:58 +0900 Subject: [PATCH 1/7] add html fixture test to test unmount behavior --- fixtures/packaging/babel-standalone/dev.html | 110 +++++++++++++++++-- 1 file changed, 99 insertions(+), 11 deletions(-) diff --git a/fixtures/packaging/babel-standalone/dev.html b/fixtures/packaging/babel-standalone/dev.html index 9c3a931badcc4..0e5906ad9eec2 100644 --- a/fixtures/packaging/babel-standalone/dev.html +++ b/fixtures/packaging/babel-standalone/dev.html @@ -1,14 +1,102 @@ - - - - -
- + + +
+
+
+ - + + \ No newline at end of file From 62e4eacc2884aa5ce47bcdd132be133e7150e69b Mon Sep 17 00:00:00 2001 From: marc hurabielle Date: Thu, 27 Sep 2018 22:25:27 +0900 Subject: [PATCH 2/7] add unit test to reproduce same behavior --- .../react-dom/src/__tests__/ReactDOM-test.js | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOM-test.js b/packages/react-dom/src/__tests__/ReactDOM-test.js index ffa9c7f7cca19..5f69119b5292d 100644 --- a/packages/react-dom/src/__tests__/ReactDOM-test.js +++ b/packages/react-dom/src/__tests__/ReactDOM-test.js @@ -525,4 +525,33 @@ describe('ReactDOM', () => { ' in App (at **)', ]); }); + + it('should not remove _reactRootContainer from a root if unmountComponentAtNode is follow by a render', () => { + const containerParent = document.createElement('div'); + const containerApp = document.createElement('div'); + + function App1() { + return ( +
+ ); + } + function App2() { + return ( +
+ ); + } + + function Parent() { + ReactDOM.render(, containerApp); + ReactDOM.unmountComponentAtNode(containerApp); + ReactDOM.render(, containerApp); + return ( +
+ ); + } + + ReactDOM.render(, containerParent); + expect(containerApp._reactRootContainer).not.toBeNull(); + expect(containerApp.innerHTML).toEqual('
'); + }); }); From 4db17021782e564d19ef235fbe1f57b76c73e341 Mon Sep 17 00:00:00 2001 From: marc hurabielle Date: Thu, 27 Sep 2018 22:27:22 +0900 Subject: [PATCH 3/7] revert fixture test infavor of unit test --- fixtures/packaging/babel-standalone/dev.html | 110 ++----------------- 1 file changed, 11 insertions(+), 99 deletions(-) diff --git a/fixtures/packaging/babel-standalone/dev.html b/fixtures/packaging/babel-standalone/dev.html index 0e5906ad9eec2..9c3a931badcc4 100644 --- a/fixtures/packaging/babel-standalone/dev.html +++ b/fixtures/packaging/babel-standalone/dev.html @@ -1,102 +1,14 @@ - - - - - -
-
-
- + + +
+ - - + \ No newline at end of file From 2ee1335f02469a16166e27e89f09efb9e17ded5b Mon Sep 17 00:00:00 2001 From: marc hurabielle Date: Thu, 27 Sep 2018 22:29:33 +0900 Subject: [PATCH 4/7] add quick prototype fix: check if container is empty before set to null --- packages/react-dom/src/client/ReactDOM.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 80029c4fcc983..ceb488940dc8d 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -681,7 +681,9 @@ const ReactDOM: Object = { // Unmount should not be batched. DOMRenderer.unbatchedUpdates(() => { legacyRenderSubtreeIntoContainer(null, null, container, false, () => { - container._reactRootContainer = null; + if (!container._reactRootContainer._internalRoot.containerInfo.firstChild) { + container._reactRootContainer = null; + } }); }); // If you call unmountComponentAtNode twice in quick succession, you'll From 003225dfade563312510bedf8fce25c7a940c66c Mon Sep 17 00:00:00 2001 From: marc hurabielle Date: Thu, 27 Sep 2018 22:44:57 +0900 Subject: [PATCH 5/7] lint test --- .../react-dom/src/__tests__/ReactDOM-test.js | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOM-test.js b/packages/react-dom/src/__tests__/ReactDOM-test.js index 5f69119b5292d..b2e1caa57bc0a 100644 --- a/packages/react-dom/src/__tests__/ReactDOM-test.js +++ b/packages/react-dom/src/__tests__/ReactDOM-test.js @@ -531,27 +531,23 @@ describe('ReactDOM', () => { const containerApp = document.createElement('div'); function App1() { - return ( -
- ); + return
; } function App2() { - return ( -
- ); + return
; } function Parent() { ReactDOM.render(, containerApp); ReactDOM.unmountComponentAtNode(containerApp); ReactDOM.render(, containerApp); - return ( -
- ); + return
; } ReactDOM.render(, containerParent); + // if null, the next unmountComponentAtNode / render might not behave correctly + // skipping some life cycle like componentWillUnmount of App2 etc. expect(containerApp._reactRootContainer).not.toBeNull(); - expect(containerApp.innerHTML).toEqual('
'); + expect(containerApp.innerHTML).toEqual('
'); }); }); From 3f7306f578430fe4ede3f11811af921af5514467 Mon Sep 17 00:00:00 2001 From: marc hurabielle Date: Fri, 28 Sep 2018 00:05:32 +0900 Subject: [PATCH 6/7] add solution with mutation of root --- packages/react-dom/src/client/ReactDOM.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index ceb488940dc8d..b14144a761da1 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -564,6 +564,9 @@ function legacyRenderSubtreeIntoContainer( callback, ); } else { + if (root.shouldBeUnMount && children != null) { + root.shouldBeUnMount = false; + } root.render(children, callback); } } @@ -679,9 +682,10 @@ const ReactDOM: Object = { } // Unmount should not be batched. + container._reactRootContainer.shouldBeUnMount = true; DOMRenderer.unbatchedUpdates(() => { legacyRenderSubtreeIntoContainer(null, null, container, false, () => { - if (!container._reactRootContainer._internalRoot.containerInfo.firstChild) { + if (container._reactRootContainer.shouldBeUnMount) { container._reactRootContainer = null; } }); From 4bdf2e78245430f6989c8804ca55e9826b793905 Mon Sep 17 00:00:00 2001 From: marc hurabielle Date: Fri, 28 Sep 2018 01:13:52 +0900 Subject: [PATCH 7/7] use lifecycle where side effects are allowed like componentDidMount --- packages/react-dom/src/__tests__/ReactDOM-test.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOM-test.js b/packages/react-dom/src/__tests__/ReactDOM-test.js index b2e1caa57bc0a..57455c232671a 100644 --- a/packages/react-dom/src/__tests__/ReactDOM-test.js +++ b/packages/react-dom/src/__tests__/ReactDOM-test.js @@ -537,11 +537,16 @@ describe('ReactDOM', () => { return
; } - function Parent() { - ReactDOM.render(, containerApp); - ReactDOM.unmountComponentAtNode(containerApp); - ReactDOM.render(, containerApp); - return
; + class Parent extends React.Component { + render() { + return
; + } + + componentDidMount() { + ReactDOM.render(, containerApp); + ReactDOM.unmountComponentAtNode(containerApp); + ReactDOM.render(, containerApp); + } } ReactDOM.render(, containerParent);