From 4482fddeda166b0ce4ff3e4f1d8f8b6d9b26179c Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 1 Apr 2019 19:33:39 +0100 Subject: [PATCH] Fix host context issues around EventComponents and EventTargets (#15284) --- .../src/ReactFiberBeginWork.js | 9 +- .../src/ReactFiberUnwindWork.js | 9 + .../ReactFiberEvents-test-internal.js | 534 ++++++++++++++++++ 3 files changed, 551 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 3c5e0fddae0e1..a2eea421d4ab2 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -2112,8 +2112,15 @@ function beginWork( // been unsuspended it has committed as a regular Suspense component. // If it needs to be retried, it should have work scheduled on it. workInProgress.effectTag |= DidCapture; - break; } + break; + } + case EventComponent: + case EventTarget: { + if (enableEventAPI) { + pushHostContextForEvent(workInProgress); + } + break; } } return bailoutOnAlreadyFinishedWork( diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index 2d76a3b6b5307..006cc5e749029 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -27,6 +27,8 @@ import { SuspenseComponent, DehydratedSuspenseComponent, IncompleteClassComponent, + EventComponent, + EventTarget, } from 'shared/ReactWorkTags'; import { DidCapture, @@ -38,6 +40,7 @@ import { import { enableSchedulerTracing, enableSuspenseServerRenderer, + enableEventAPI, } from 'shared/ReactFeatureFlags'; import {ConcurrentMode} from './ReactTypeOfMode'; import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent'; @@ -510,6 +513,12 @@ function unwindWork( case ContextProvider: popProvider(workInProgress); return null; + case EventComponent: + case EventTarget: + if (enableEventAPI) { + popHostContext(workInProgress); + } + return null; default: return null; } diff --git a/packages/react-reconciler/src/__tests__/ReactFiberEvents-test-internal.js b/packages/react-reconciler/src/__tests__/ReactFiberEvents-test-internal.js index 3cfed203ad4c7..9a697379d3bc7 100644 --- a/packages/react-reconciler/src/__tests__/ReactFiberEvents-test-internal.js +++ b/packages/react-reconciler/src/__tests__/ReactFiberEvents-test-internal.js @@ -17,6 +17,7 @@ let EventComponent; let ReactTestRenderer; let ReactDOM; let ReactDOMServer; +let ReactTestUtils; let EventTarget; let ReactEvents; @@ -55,6 +56,7 @@ function initTestRenderer() { function initReactDOM() { init(); ReactDOM = require('react-dom'); + ReactTestUtils = require('react-dom/test-utils'); } function initReactDOMServer() { @@ -219,6 +221,188 @@ describe('ReactFiberEvents', () => { 'Warning: validateDOMNesting: React event targets must not have event components as children.', ); }); + + it('should handle event components correctly with error boundaries', () => { + function ErrorComponent() { + throw new Error('Failed!'); + } + + const Test = () => ( + + + + + + + + ); + + class Wrapper extends React.Component { + state = { + error: null, + }; + + componentDidCatch(error) { + this.setState({ + error, + }); + } + + render() { + if (this.state.error) { + return 'Worked!'; + } + return ; + } + } + + ReactNoop.render(); + expect(Scheduler).toFlushWithoutYielding(); + expect(ReactNoop).toMatchRenderedOutput('Worked!'); + }); + + it('should handle re-renders where there is a bail-out in a parent', () => { + let _updateCounter; + + function Child() { + const [counter, updateCounter] = React.useState(0); + + _updateCounter = updateCounter; + + return ( +
+ Child - {counter} +
+ ); + } + + const Parent = () => ( + + +
+ +
+
+
+ ); + + ReactNoop.render(); + expect(Scheduler).toFlushWithoutYielding(); + expect(ReactNoop).toMatchRenderedOutput( +
+
+ Child - 0 +
+
, + ); + + ReactNoop.act(() => { + _updateCounter(counter => counter + 1); + }); + expect(Scheduler).toFlushWithoutYielding(); + + expect(ReactNoop).toMatchRenderedOutput( +
+
+ Child - 1 +
+
, + ); + }); + + it('should handle re-renders where there is a bail-out in a parent and an error occurs', () => { + let _updateCounter; + + function Child() { + const [counter, updateCounter] = React.useState(0); + + _updateCounter = updateCounter; + + if (counter === 1) { + return null; + } + + return ( +
+ Child - {counter} +
+ ); + } + + const Parent = () => ( + + + + + + ); + + ReactNoop.render(); + expect(Scheduler).toFlushWithoutYielding(); + expect(ReactNoop).toMatchRenderedOutput( +
+ Child - 0 +
, + ); + + expect(() => { + ReactNoop.act(() => { + _updateCounter(counter => counter + 1); + }); + expect(Scheduler).toFlushWithoutYielding(); + }).toWarnDev( + 'Warning: must have a single DOM element as a child. Found no children.', + ); + }); + + it('should handle re-renders where there is a bail-out in a parent and an error occurs #2', () => { + let _updateCounter; + + function Child() { + const [counter, updateCounter] = React.useState(0); + + _updateCounter = updateCounter; + + if (counter === 1) { + return ( + +
Child
+
+ ); + } + + return ( +
+ Child - {counter} +
+ ); + } + + const Parent = () => ( + + + + + + ); + + ReactNoop.render(); + expect(Scheduler).toFlushWithoutYielding(); + expect(ReactNoop).toMatchRenderedOutput( +
+ Child - 0 +
, + ); + + expect(() => { + ReactNoop.act(() => { + _updateCounter(counter => counter + 1); + }); + expect(Scheduler).toFlushWithoutYielding(); + }).toWarnDev( + 'Warning: validateDOMNesting: React event targets must not have event components as children.', + ); + }); }); describe('TestRenderer', () => { @@ -407,6 +591,190 @@ describe('ReactFiberEvents', () => { 'Warning: validateDOMNesting: React event targets must not have event components as children.', ); }); + + it('should handle event components correctly with error boundaries', () => { + function ErrorComponent() { + throw new Error('Failed!'); + } + + const Test = () => ( + + + + + + + + ); + + class Wrapper extends React.Component { + state = { + error: null, + }; + + componentDidCatch(error) { + this.setState({ + error, + }); + } + + render() { + if (this.state.error) { + return 'Worked!'; + } + return ; + } + } + + const root = ReactTestRenderer.create(null); + root.update(); + expect(Scheduler).toFlushWithoutYielding(); + expect(root).toMatchRenderedOutput('Worked!'); + }); + + it('should handle re-renders where there is a bail-out in a parent', () => { + let _updateCounter; + + function Child() { + const [counter, updateCounter] = React.useState(0); + + _updateCounter = updateCounter; + + return ( +
+ Child - {counter} +
+ ); + } + + const Parent = () => ( + + +
+ +
+
+
+ ); + + const root = ReactTestRenderer.create(null); + root.update(); + expect(Scheduler).toFlushWithoutYielding(); + expect(root).toMatchRenderedOutput( +
+
+ Child - 0 +
+
, + ); + + ReactTestRenderer.act(() => { + _updateCounter(counter => counter + 1); + }); + + expect(root).toMatchRenderedOutput( +
+
+ Child - 1 +
+
, + ); + }); + + it('should handle re-renders where there is a bail-out in a parent and an error occurs', () => { + let _updateCounter; + + function Child() { + const [counter, updateCounter] = React.useState(0); + + _updateCounter = updateCounter; + + if (counter === 1) { + return null; + } + + return ( +
+ Child - {counter} +
+ ); + } + + const Parent = () => ( + + + + + + ); + + const root = ReactTestRenderer.create(null); + root.update(); + expect(Scheduler).toFlushWithoutYielding(); + expect(root).toMatchRenderedOutput( +
+ Child - 0 +
, + ); + + expect(() => { + ReactTestRenderer.act(() => { + _updateCounter(counter => counter + 1); + }); + }).toWarnDev( + 'Warning: must have a single DOM element as a child. Found no children.', + ); + }); + + it('should handle re-renders where there is a bail-out in a parent and an error occurs #2', () => { + let _updateCounter; + + function Child() { + const [counter, updateCounter] = React.useState(0); + + _updateCounter = updateCounter; + + if (counter === 1) { + return ( + +
Child
+
+ ); + } + + return ( +
+ Child - {counter} +
+ ); + } + + const Parent = () => ( + + + + + + ); + + const root = ReactTestRenderer.create(null); + root.update(); + expect(Scheduler).toFlushWithoutYielding(); + expect(root).toMatchRenderedOutput( +
+ Child - 0 +
, + ); + + expect(() => { + ReactTestRenderer.act(() => { + _updateCounter(counter => counter + 1); + }); + expect(Scheduler).toFlushWithoutYielding(); + }).toWarnDev( + 'Warning: validateDOMNesting: React event targets must not have event components as children.', + ); + }); }); describe('ReactDOM', () => { @@ -594,6 +962,172 @@ describe('ReactFiberEvents', () => { 'Warning: validateDOMNesting: React event targets must not have event components as children.', ); }); + + it('should handle event components correctly with error boundaries', () => { + function ErrorComponent() { + throw new Error('Failed!'); + } + + const Test = () => ( + + + + + + + + ); + + class Wrapper extends React.Component { + state = { + error: null, + }; + + componentDidCatch(error) { + this.setState({ + error, + }); + } + + render() { + if (this.state.error) { + return 'Worked!'; + } + return ; + } + } + + const container = document.createElement('div'); + ReactDOM.render(, container); + expect(Scheduler).toFlushWithoutYielding(); + expect(container.innerHTML).toBe('Worked!'); + }); + + it('should handle re-renders where there is a bail-out in a parent', () => { + let _updateCounter; + + function Child() { + const [counter, updateCounter] = React.useState(0); + + _updateCounter = updateCounter; + + return ( +
+ Child - {counter} +
+ ); + } + + const Parent = () => ( + + +
+ +
+
+
+ ); + + const container = document.createElement('div'); + ReactDOM.render(, container); + expect(container.innerHTML).toBe( + '
Child - 0
', + ); + + ReactTestUtils.act(() => { + _updateCounter(counter => counter + 1); + }); + + expect(container.innerHTML).toBe( + '
Child - 1
', + ); + }); + + it('should handle re-renders where there is a bail-out in a parent and an error occurs', () => { + let _updateCounter; + + function Child() { + const [counter, updateCounter] = React.useState(0); + + _updateCounter = updateCounter; + + if (counter === 1) { + return null; + } + + return ( +
+ Child - {counter} +
+ ); + } + + const Parent = () => ( + + + + + + ); + + const container = document.createElement('div'); + ReactDOM.render(, container); + expect(container.innerHTML).toBe('
Child - 0
'); + + expect(() => { + ReactTestUtils.act(() => { + _updateCounter(counter => counter + 1); + }); + expect(Scheduler).toFlushWithoutYielding(); + }).toWarnDev( + 'Warning: must have a single DOM element as a child. Found no children.', + ); + }); + + it('should handle re-renders where there is a bail-out in a parent and an error occurs #2', () => { + let _updateCounter; + + function Child() { + const [counter, updateCounter] = React.useState(0); + + _updateCounter = updateCounter; + + if (counter === 1) { + return ( + +
Child
+
+ ); + } + + return ( +
+ Child - {counter} +
+ ); + } + + const Parent = () => ( + + + + + + ); + + const container = document.createElement('div'); + ReactDOM.render(, container); + expect(container.innerHTML).toBe('
Child - 0
'); + + expect(() => { + ReactTestUtils.act(() => { + _updateCounter(counter => counter + 1); + }); + expect(Scheduler).toFlushWithoutYielding(); + }).toWarnDev( + 'Warning: validateDOMNesting: React event targets must not have event components as children.', + ); + }); }); describe('ReactDOMServer', () => {