Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove buggy unstable_deferredUpdates() #13488

Merged
merged 4 commits into from
Aug 28, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might look like nitpicking, since it's completely unrelated to the issue at hand, but I'm honestly curious on why you used MouseEvent for input here.

Sorry for the random chime in 👐

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I used click at first and then forgot to change when I rewrote the test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks for the quick answer :)~

// 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 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to remove it from the scheduler/noop too?

const currentTime = requestCurrentTime();
const previousExpirationContext = expirationContext;
const previousIsBatchingInteractiveUpdates = isBatchingInteractiveUpdates;
expirationContext = computeAsyncExpiration(currentTime);
isBatchingInteractiveUpdates = false;
try {
return fn();
} finally {
expirationContext = previousExpirationContext;
isBatchingInteractiveUpdates = previousIsBatchingInteractiveUpdates;
}
}

Expand Down