Skip to content

Commit

Permalink
Remove buggy unstable_deferredUpdates() (facebook#13488)
Browse files Browse the repository at this point in the history
* Add a failing test for deferredUpdates not being respected

* Don't consider deferred updates interactive

* Remove now-unnecessary setTimeout hack in the fixture

* Remove unstable_deferredUpdates
  • Loading branch information
gaearon authored and jetoneza committed Jan 23, 2019
1 parent e370ff1 commit 81da909
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 34 deletions.
3 changes: 1 addition & 2 deletions fixtures/unstable-async/suspense/src/components/App.js
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -31,7 +30,7 @@ export default class App extends PureComponent {
this.setState({
currentId: id,
});
unstable_deferredUpdates(() => {
requestIdleCallback(() => {
this.setState({
showDetail: true,
});
Expand Down
53 changes: 25 additions & 28 deletions fixtures/unstable-async/time-slicing/src/index.js
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -54,22 +54,21 @@ 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) {
return;
}
this._ignoreClick = true;

// TODO: needing setTimeout here seems like a React bug.
setTimeout(() => {
unstable_deferredUpdates(() => {
this.setState({showDemo: true}, () => {
this._ignoreClick = false;
});
requestIdleCallback(() => {
this.setState({showDemo: true}, () => {
this._ignoreClick = false;
});
});
};
Expand Down Expand Up @@ -107,11 +106,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});
});
requestIdleCallback(() => {
this.setState({value});
});
break;
default:
Expand All @@ -120,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 (
Expand All @@ -137,20 +131,23 @@ class App extends PureComponent {
defaultValue={this.state.input}
onChange={this.handleChange}
/>
<Wrapper>
<div className="demo" onClick={this.handleChartClick}>
{this.state.showDemo && (
<Charts data={data} onClick={this.handleChartClick} />
)}
<div style={{display: showClock ? 'block' : 'none'}}>
<Clock />
</div>
<div className="demo" onClick={this.handleChartClick}>
{this.state.showDemo && (
<Charts data={data} onClick={this.handleChartClick} />
)}
<div style={{display: showClock ? 'block' : 'none'}}>
<Clock />
</div>
</Wrapper>
</div>
</div>
);
}
}

const container = document.getElementById('root');
render(<App />, container);
render(
<React.unstable_AsyncMode>
<App />
</React.unstable_AsyncMode>,
container
);
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ let ReactDOM;

const AsyncMode = React.unstable_AsyncMode;

const setUntrackedInputValue = Object.getOwnPropertyDescriptor(
HTMLInputElement.prototype,
'value',
).set;

describe('ReactDOMFiberAsync', () => {
let container;

Expand Down Expand Up @@ -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', () => {
Expand All @@ -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;
requestIdleCallback(() => {
this.setState({
asyncValue: nextValue,
});
});
this.setState({
syncValue: nextValue,
});
};

render() {
return (
<div>
<input
ref={inputRef}
onChange={this.handleChange}
defaultValue=""
/>
<p ref={asyncValueRef}>{this.state.asyncValue}</p>
<p ref={syncValueRef}>{this.state.syncValue}</p>
</div>
);
}
}
ReactDOM.render(<Counter />, 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');
});

Expand All @@ -91,7 +151,6 @@ describe('ReactDOMFiberAsync', () => {
beforeEach(() => {
jest.resetModules();
ReactFeatureFlags = require('shared/ReactFeatureFlags');
container = document.createElement('div');
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
ReactDOM = require('react-dom');
});
Expand Down
2 changes: 0 additions & 2 deletions packages/react-dom/src/client/ReactDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -732,8 +732,6 @@ const ReactDOM: Object = {

unstable_batchedUpdates: DOMRenderer.batchedUpdates,

unstable_deferredUpdates: DOMRenderer.deferredUpdates,

unstable_interactiveUpdates: DOMRenderer.interactiveUpdates,

flushSync: DOMRenderer.flushSync,
Expand Down
3 changes: 3 additions & 0 deletions packages/react-reconciler/src/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -1568,11 +1568,14 @@ function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) {
function deferredUpdates<A>(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;
}
}

Expand Down

0 comments on commit 81da909

Please sign in to comment.