From f9c95effb2f21f784bd8e7c9f66a2062837337e2 Mon Sep 17 00:00:00 2001 From: Dan Date: Sun, 26 Aug 2018 17:46:48 +0100 Subject: [PATCH 1/4] Add a failing test for deferredUpdates not being respected --- .../ReactDOMFiberAsync-test.internal.js | 63 ++++++++++++++++++- 1 file changed, 61 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.internal.js index 51b901f1dd58c..3d22076228d31 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.internal.js @@ -16,6 +16,11 @@ let ReactDOM; const AsyncMode = React.unstable_AsyncMode; +const setUntrackedInputValue = Object.getOwnPropertyDescriptor( + HTMLInputElement.prototype, + 'value', +).set; + describe('ReactDOMFiberAsync', () => { let container; @@ -47,6 +52,12 @@ describe('ReactDOMFiberAsync', () => { jest.resetModules(); container = document.createElement('div'); ReactDOM = require('react-dom'); + + document.body.appendChild(container); + }); + + afterEach(() => { + document.body.removeChild(container); }); it('renders synchronously by default', () => { @@ -60,11 +71,60 @@ describe('ReactDOMFiberAsync', () => { expect(ops).toEqual(['Hi', 'Bye']); }); + it('does not perform deferred updates synchronously', () => { + let inputRef = React.createRef(); + let asyncValueRef = React.createRef(); + let syncValueRef = React.createRef(); + + class Counter extends React.Component { + state = {asyncValue: '', syncValue: ''}; + + handleChange = e => { + const nextValue = e.target.value; + ReactDOM.unstable_deferredUpdates(() => { + this.setState({ + asyncValue: nextValue, + }); + }); + this.setState({ + syncValue: nextValue, + }); + }; + + render() { + return ( +
+ +

{this.state.asyncValue}

+

{this.state.syncValue}

+
+ ); + } + } + ReactDOM.render(, container); + expect(asyncValueRef.current.textContent).toBe(''); + expect(syncValueRef.current.textContent).toBe(''); + + setUntrackedInputValue.call(inputRef.current, 'hello'); + inputRef.current.dispatchEvent(new MouseEvent('input', {bubbles: true})); + // Should only flush non-deferred update. + expect(asyncValueRef.current.textContent).toBe(''); + expect(syncValueRef.current.textContent).toBe('hello'); + + // Should flush both updates now. + jest.runAllTimers(); + expect(asyncValueRef.current.textContent).toBe('hello'); + expect(syncValueRef.current.textContent).toBe('hello'); + }); + describe('with feature flag disabled', () => { beforeEach(() => { jest.resetModules(); ReactFeatureFlags = require('shared/ReactFeatureFlags'); - container = document.createElement('div'); ReactDOM = require('react-dom'); }); @@ -91,7 +151,6 @@ describe('ReactDOMFiberAsync', () => { beforeEach(() => { jest.resetModules(); ReactFeatureFlags = require('shared/ReactFeatureFlags'); - container = document.createElement('div'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; ReactDOM = require('react-dom'); }); From 0a14e3b99f5a8233710fb564966452dbb2c11dc3 Mon Sep 17 00:00:00 2001 From: Dan Date: Mon, 27 Aug 2018 15:13:18 +0100 Subject: [PATCH 2/4] Don't consider deferred updates interactive --- packages/react-reconciler/src/ReactFiberScheduler.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 10161f42e4589..29f39a13cb8ab 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1568,11 +1568,14 @@ function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) { function deferredUpdates(fn: () => A): A { const currentTime = requestCurrentTime(); const previousExpirationContext = expirationContext; + const previousIsBatchingInteractiveUpdates = isBatchingInteractiveUpdates; expirationContext = computeAsyncExpiration(currentTime); + isBatchingInteractiveUpdates = false; try { return fn(); } finally { expirationContext = previousExpirationContext; + isBatchingInteractiveUpdates = previousIsBatchingInteractiveUpdates; } } From ebdca9f8c240b3f86ef1cd334fce31911970aee3 Mon Sep 17 00:00:00 2001 From: Dan Date: Mon, 27 Aug 2018 15:20:08 +0100 Subject: [PATCH 3/4] Remove now-unnecessary setTimeout hack in the fixture --- .../unstable-async/time-slicing/src/index.js | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/fixtures/unstable-async/time-slicing/src/index.js b/fixtures/unstable-async/time-slicing/src/index.js index c7f666e5a16e6..8bf23dac134c5 100644 --- a/fixtures/unstable-async/time-slicing/src/index.js +++ b/fixtures/unstable-async/time-slicing/src/index.js @@ -64,12 +64,9 @@ class App extends PureComponent { } this._ignoreClick = true; - // TODO: needing setTimeout here seems like a React bug. - setTimeout(() => { - unstable_deferredUpdates(() => { - this.setState({showDemo: true}, () => { - this._ignoreClick = false; - }); + unstable_deferredUpdates(() => { + this.setState({showDemo: true}, () => { + this._ignoreClick = false; }); }); }; @@ -107,11 +104,8 @@ class App extends PureComponent { this.debouncedHandleChange(value); break; case 'async': - // TODO: needing setTimeout here seems like a React bug. - setTimeout(() => { - unstable_deferredUpdates(() => { - this.setState({value}); - }); + unstable_deferredUpdates(() => { + this.setState({value}); }); break; default: From 803ee47d0cb2c5479cbfc3795774b344a1905557 Mon Sep 17 00:00:00 2001 From: Dan Date: Mon, 27 Aug 2018 21:44:53 +0100 Subject: [PATCH 4/4] Remove unstable_deferredUpdates --- .../suspense/src/components/App.js | 3 +- .../unstable-async/time-slicing/src/index.js | 41 ++++++++++--------- .../ReactDOMFiberAsync-test.internal.js | 2 +- packages/react-dom/src/client/ReactDOM.js | 2 - 4 files changed, 24 insertions(+), 24 deletions(-) diff --git a/fixtures/unstable-async/suspense/src/components/App.js b/fixtures/unstable-async/suspense/src/components/App.js index 1c8df3c1092b1..a943f54965a03 100644 --- a/fixtures/unstable-async/suspense/src/components/App.js +++ b/fixtures/unstable-async/suspense/src/components/App.js @@ -1,5 +1,4 @@ import React, {Placeholder, PureComponent} from 'react'; -import {unstable_deferredUpdates} from 'react-dom'; import {createResource} from 'simple-cache-provider'; import {cache} from '../cache'; import Spinner from './Spinner'; @@ -31,7 +30,7 @@ export default class App extends PureComponent { this.setState({ currentId: id, }); - unstable_deferredUpdates(() => { + requestIdleCallback(() => { this.setState({ showDetail: true, }); diff --git a/fixtures/unstable-async/time-slicing/src/index.js b/fixtures/unstable-async/time-slicing/src/index.js index 8bf23dac134c5..104bf9f7dd1bf 100644 --- a/fixtures/unstable-async/time-slicing/src/index.js +++ b/fixtures/unstable-async/time-slicing/src/index.js @@ -1,5 +1,5 @@ -import React, {PureComponent, unstable_AsyncMode} from 'react'; -import {flushSync, render, unstable_deferredUpdates} from 'react-dom'; +import React, {PureComponent} from 'react'; +import {flushSync, render} from 'react-dom'; import _ from 'lodash'; import Charts from './Charts'; import Clock from './Clock'; @@ -54,9 +54,11 @@ class App extends PureComponent { return; } if (this.state.strategy !== 'async') { - this.setState(state => ({ - showDemo: !state.showDemo, - })); + flushSync(() => { + this.setState(state => ({ + showDemo: !state.showDemo, + })); + }); return; } if (this._ignoreClick) { @@ -64,7 +66,7 @@ class App extends PureComponent { } this._ignoreClick = true; - unstable_deferredUpdates(() => { + requestIdleCallback(() => { this.setState({showDemo: true}, () => { this._ignoreClick = false; }); @@ -104,7 +106,7 @@ class App extends PureComponent { this.debouncedHandleChange(value); break; case 'async': - unstable_deferredUpdates(() => { + requestIdleCallback(() => { this.setState({value}); }); break; @@ -114,8 +116,6 @@ class App extends PureComponent { }; render() { - const Wrapper = - this.state.strategy === 'async' ? unstable_AsyncMode : 'div'; const {showClock} = this.state; const data = this.getStreamData(this.state.value); return ( @@ -131,20 +131,23 @@ class App extends PureComponent { defaultValue={this.state.input} onChange={this.handleChange} /> - -
- {this.state.showDemo && ( - - )} -
- -
+
+ {this.state.showDemo && ( + + )} +
+
- +
); } } const container = document.getElementById('root'); -render(, container); +render( + + + , + container +); diff --git a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.internal.js index 3d22076228d31..36bf2f007eae9 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.internal.js @@ -81,7 +81,7 @@ describe('ReactDOMFiberAsync', () => { handleChange = e => { const nextValue = e.target.value; - ReactDOM.unstable_deferredUpdates(() => { + requestIdleCallback(() => { this.setState({ asyncValue: nextValue, }); diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index ea887f774c154..6455f85547ddf 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -732,8 +732,6 @@ const ReactDOM: Object = { unstable_batchedUpdates: DOMRenderer.batchedUpdates, - unstable_deferredUpdates: DOMRenderer.deferredUpdates, - unstable_interactiveUpdates: DOMRenderer.interactiveUpdates, flushSync: DOMRenderer.flushSync,