From 71d012ecd07baef6f53d02bebd720794f75266ca Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 7 Oct 2019 14:15:15 -0700 Subject: [PATCH] Remove dormant createBatch experiment (#17035) * Remove dormant createBatch experiment In a hybrid React app with multiple roots, `createBatch` is used to coordinate an update to a root with its imperative container. We've pivoted away from multi-root, hybrid React apps for now to focus on single root apps. This PR removes the API from the codebase. It's possible we'll add back some version of this feature in the future. * Remove unused export --- .../src/__tests__/ReactDOMRoot-test.js | 223 --------------- packages/react-dom/src/client/ReactDOM.js | 260 +----------------- .../DOMEventResponderSystem-test.internal.js | 18 +- .../src/createReactNoop.js | 26 -- .../src/ReactFiberHotReloading.js | 6 +- .../src/ReactFiberReconciler.js | 155 ++++------- .../react-reconciler/src/ReactFiberRoot.js | 13 - .../src/ReactFiberWorkLoop.js | 52 +--- .../__tests__/ReactProfiler-test.internal.js | 74 ----- .../ReactProfilerDOM-test.internal.js | 138 ++++------ 10 files changed, 150 insertions(+), 815 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMRoot-test.js b/packages/react-dom/src/__tests__/ReactDOMRoot-test.js index 75fca7920834..532030d0b262 100644 --- a/packages/react-dom/src/__tests__/ReactDOMRoot-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMRoot-test.js @@ -43,35 +43,6 @@ describe('ReactDOMRoot', () => { expect(container.textContent).toEqual(''); }); - it('`root.render` returns a thenable work object', () => { - const root = ReactDOM.unstable_createRoot(container); - const work = root.render('Hi'); - let ops = []; - work.then(() => { - ops.push('inside callback: ' + container.textContent); - }); - ops.push('before committing: ' + container.textContent); - Scheduler.unstable_flushAll(); - ops.push('after committing: ' + container.textContent); - expect(ops).toEqual([ - 'before committing: ', - // `then` callback should fire during commit phase - 'inside callback: Hi', - 'after committing: Hi', - ]); - }); - - it('resolves `work.then` callback synchronously if the work already committed', () => { - const root = ReactDOM.unstable_createRoot(container); - const work = root.render('Hi'); - Scheduler.unstable_flushAll(); - let ops = []; - work.then(() => { - ops.push('inside callback'); - }); - expect(ops).toEqual(['inside callback']); - }); - it('supports hydration', async () => { const markup = await new Promise(resolve => resolve( @@ -129,200 +100,6 @@ describe('ReactDOMRoot', () => { expect(container.textContent).toEqual('abdc'); }); - it('can defer a commit by batching it', () => { - const root = ReactDOM.unstable_createRoot(container); - const batch = root.createBatch(); - batch.render(
Hi
); - // Hasn't committed yet - expect(container.textContent).toEqual(''); - // Commit - batch.commit(); - expect(container.textContent).toEqual('Hi'); - }); - - it('applies setState in componentDidMount synchronously in a batch', done => { - class App extends React.Component { - state = {mounted: false}; - componentDidMount() { - this.setState({ - mounted: true, - }); - } - render() { - return this.state.mounted ? 'Hi' : 'Bye'; - } - } - - const root = ReactDOM.unstable_createRoot(container); - const batch = root.createBatch(); - batch.render(); - - Scheduler.unstable_flushAll(); - - // Hasn't updated yet - expect(container.textContent).toEqual(''); - - let ops = []; - batch.then(() => { - // Still hasn't updated - ops.push(container.textContent); - - // Should synchronously commit - batch.commit(); - ops.push(container.textContent); - - expect(ops).toEqual(['', 'Hi']); - done(); - }); - }); - - it('does not restart a completed batch when committing if there were no intervening updates', () => { - let ops = []; - function Foo(props) { - ops.push('Foo'); - return props.children; - } - const root = ReactDOM.unstable_createRoot(container); - const batch = root.createBatch(); - batch.render(Hi); - // Flush all async work. - Scheduler.unstable_flushAll(); - // Root should complete without committing. - expect(ops).toEqual(['Foo']); - expect(container.textContent).toEqual(''); - - ops = []; - - // Commit. Shouldn't re-render Foo. - batch.commit(); - expect(ops).toEqual([]); - expect(container.textContent).toEqual('Hi'); - }); - - it('can wait for a batch to finish', () => { - const root = ReactDOM.unstable_createRoot(container); - const batch = root.createBatch(); - batch.render('Foo'); - - Scheduler.unstable_flushAll(); - - // Hasn't updated yet - expect(container.textContent).toEqual(''); - - let ops = []; - batch.then(() => { - // Still hasn't updated - ops.push(container.textContent); - // Should synchronously commit - batch.commit(); - ops.push(container.textContent); - }); - - expect(ops).toEqual(['', 'Foo']); - }); - - it('`batch.render` returns a thenable work object', () => { - const root = ReactDOM.unstable_createRoot(container); - const batch = root.createBatch(); - const work = batch.render('Hi'); - let ops = []; - work.then(() => { - ops.push('inside callback: ' + container.textContent); - }); - ops.push('before committing: ' + container.textContent); - batch.commit(); - ops.push('after committing: ' + container.textContent); - expect(ops).toEqual([ - 'before committing: ', - // `then` callback should fire during commit phase - 'inside callback: Hi', - 'after committing: Hi', - ]); - }); - - it('can commit an empty batch', () => { - const root = ReactDOM.unstable_createRoot(container); - root.render(1); - - Scheduler.unstable_advanceTime(2000); - // This batch has a later expiration time than the earlier update. - const batch = root.createBatch(); - - // This should not flush the earlier update. - batch.commit(); - expect(container.textContent).toEqual(''); - - Scheduler.unstable_flushAll(); - expect(container.textContent).toEqual('1'); - }); - - it('two batches created simultaneously are committed separately', () => { - // (In other words, they have distinct expiration times) - const root = ReactDOM.unstable_createRoot(container); - const batch1 = root.createBatch(); - batch1.render(1); - const batch2 = root.createBatch(); - batch2.render(2); - - expect(container.textContent).toEqual(''); - - batch1.commit(); - expect(container.textContent).toEqual('1'); - - batch2.commit(); - expect(container.textContent).toEqual('2'); - }); - - it('commits an earlier batch without committing a later batch', () => { - const root = ReactDOM.unstable_createRoot(container); - const batch1 = root.createBatch(); - batch1.render(1); - - // This batch has a later expiration time - Scheduler.unstable_advanceTime(2000); - const batch2 = root.createBatch(); - batch2.render(2); - - expect(container.textContent).toEqual(''); - - batch1.commit(); - expect(container.textContent).toEqual('1'); - - batch2.commit(); - expect(container.textContent).toEqual('2'); - }); - - it('commits a later batch without committing an earlier batch', () => { - const root = ReactDOM.unstable_createRoot(container); - const batch1 = root.createBatch(); - batch1.render(1); - - // This batch has a later expiration time - Scheduler.unstable_advanceTime(2000); - const batch2 = root.createBatch(); - batch2.render(2); - - expect(container.textContent).toEqual(''); - - batch2.commit(); - expect(container.textContent).toEqual('2'); - - batch1.commit(); - Scheduler.unstable_flushAll(); - expect(container.textContent).toEqual('1'); - }); - - it('handles fatal errors triggered by batch.commit()', () => { - const root = ReactDOM.unstable_createRoot(container); - const batch = root.createBatch(); - const InvalidType = undefined; - expect(() => batch.render()).toWarnDev( - ['React.createElement: type is invalid'], - {withoutStack: true}, - ); - expect(() => batch.commit()).toThrow('Element type is invalid'); - }); - it('throws a good message on invalid containers', () => { expect(() => { ReactDOM.unstable_createRoot(
Hi
); diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 753601dbecdd..37f67e0ce853 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -11,19 +11,13 @@ import type {ReactNodeList} from 'shared/ReactTypes'; import type {RootTag} from 'shared/ReactRootTags'; // TODO: This type is shared between the reconciler and ReactDOM, but will // eventually be lifted out to the renderer. -import type { - FiberRoot, - Batch as FiberRootBatch, -} from 'react-reconciler/src/ReactFiberRoot'; +import type {FiberRoot} from 'react-reconciler/src/ReactFiberRoot'; import '../shared/checkReact'; import './ReactDOMClientInjection'; import { - computeUniqueAsyncExpiration, findHostInstanceWithNoPortals, - updateContainerAtExpirationTime, - flushRoot, createContainer, updateContainer, batchedEventUpdates, @@ -179,209 +173,21 @@ setRestoreImplementation(restoreControlledState); export type DOMContainer = | (Element & { - _reactRootContainer: ?(_ReactRoot | _ReactSyncRoot), + _reactRootContainer: ?_ReactRoot, _reactHasBeenPassedToCreateRootDEV: ?boolean, }) | (Document & { - _reactRootContainer: ?(_ReactRoot | _ReactSyncRoot), + _reactRootContainer: ?_ReactRoot, _reactHasBeenPassedToCreateRootDEV: ?boolean, }); -type Batch = FiberRootBatch & { - render(children: ReactNodeList): Work, - then(onComplete: () => mixed): void, - commit(): void, - - // The ReactRoot constructor is hoisted but the prototype methods are not. If - // we move ReactRoot to be above ReactBatch, the inverse error occurs. - // $FlowFixMe Hoisting issue. - _root: _ReactRoot | _ReactSyncRoot, - _hasChildren: boolean, - _children: ReactNodeList, - - _callbacks: Array<() => mixed> | null, - _didComplete: boolean, -}; - -type _ReactSyncRoot = { - render(children: ReactNodeList, callback: ?() => mixed): Work, - unmount(callback: ?() => mixed): Work, +type _ReactRoot = { + render(children: ReactNodeList, callback: ?() => mixed): void, + unmount(callback: ?() => mixed): void, _internalRoot: FiberRoot, }; -type _ReactRoot = _ReactSyncRoot & { - createBatch(): Batch, -}; - -function ReactBatch(root: _ReactRoot | _ReactSyncRoot) { - const expirationTime = computeUniqueAsyncExpiration(); - this._expirationTime = expirationTime; - this._root = root; - this._next = null; - this._callbacks = null; - this._didComplete = false; - this._hasChildren = false; - this._children = null; - this._defer = true; -} -ReactBatch.prototype.render = function(children: ReactNodeList) { - invariant( - this._defer, - 'batch.render: Cannot render a batch that already committed.', - ); - this._hasChildren = true; - this._children = children; - const internalRoot = this._root._internalRoot; - const expirationTime = this._expirationTime; - const work = new ReactWork(); - updateContainerAtExpirationTime( - children, - internalRoot, - null, - expirationTime, - null, - work._onCommit, - ); - return work; -}; -ReactBatch.prototype.then = function(onComplete: () => mixed) { - if (this._didComplete) { - onComplete(); - return; - } - let callbacks = this._callbacks; - if (callbacks === null) { - callbacks = this._callbacks = []; - } - callbacks.push(onComplete); -}; -ReactBatch.prototype.commit = function() { - const internalRoot = this._root._internalRoot; - let firstBatch = internalRoot.firstBatch; - invariant( - this._defer && firstBatch !== null, - 'batch.commit: Cannot commit a batch multiple times.', - ); - - if (!this._hasChildren) { - // This batch is empty. Return. - this._next = null; - this._defer = false; - return; - } - - let expirationTime = this._expirationTime; - - // Ensure this is the first batch in the list. - if (firstBatch !== this) { - // This batch is not the earliest batch. We need to move it to the front. - // Update its expiration time to be the expiration time of the earliest - // batch, so that we can flush it without flushing the other batches. - if (this._hasChildren) { - expirationTime = this._expirationTime = firstBatch._expirationTime; - // Rendering this batch again ensures its children will be the final state - // when we flush (updates are processed in insertion order: last - // update wins). - // TODO: This forces a restart. Should we print a warning? - this.render(this._children); - } - - // Remove the batch from the list. - let previous = null; - let batch = firstBatch; - while (batch !== this) { - previous = batch; - batch = batch._next; - } - invariant( - previous !== null, - 'batch.commit: Cannot commit a batch multiple times.', - ); - previous._next = batch._next; - - // Add it to the front. - this._next = firstBatch; - firstBatch = internalRoot.firstBatch = this; - } - - // Synchronously flush all the work up to this batch's expiration time. - this._defer = false; - flushRoot(internalRoot, expirationTime); - - // Pop the batch from the list. - const next = this._next; - this._next = null; - firstBatch = internalRoot.firstBatch = next; - - // Append the next earliest batch's children to the update queue. - if (firstBatch !== null && firstBatch._hasChildren) { - firstBatch.render(firstBatch._children); - } -}; -ReactBatch.prototype._onComplete = function() { - if (this._didComplete) { - return; - } - this._didComplete = true; - const callbacks = this._callbacks; - if (callbacks === null) { - return; - } - // TODO: Error handling. - for (let i = 0; i < callbacks.length; i++) { - const callback = callbacks[i]; - callback(); - } -}; - -type Work = { - then(onCommit: () => mixed): void, - _onCommit: () => void, - _callbacks: Array<() => mixed> | null, - _didCommit: boolean, -}; - -function ReactWork() { - this._callbacks = null; - this._didCommit = false; - // TODO: Avoid need to bind by replacing callbacks in the update queue with - // list of Work objects. - this._onCommit = this._onCommit.bind(this); -} -ReactWork.prototype.then = function(onCommit: () => mixed): void { - if (this._didCommit) { - onCommit(); - return; - } - let callbacks = this._callbacks; - if (callbacks === null) { - callbacks = this._callbacks = []; - } - callbacks.push(onCommit); -}; -ReactWork.prototype._onCommit = function(): void { - if (this._didCommit) { - return; - } - this._didCommit = true; - const callbacks = this._callbacks; - if (callbacks === null) { - return; - } - // TODO: Error handling. - for (let i = 0; i < callbacks.length; i++) { - const callback = callbacks[i]; - invariant( - typeof callback === 'function', - 'Invalid argument passed as callback. Expected a function. Instead ' + - 'received: %s', - callback, - ); - callback(); - } -}; - function createRootImpl( container: DOMContainer, tag: RootTag, @@ -418,64 +224,24 @@ function ReactRoot(container: DOMContainer, options: void | RootOptions) { ReactRoot.prototype.render = ReactSyncRoot.prototype.render = function( children: ReactNodeList, callback: ?() => mixed, -): Work { +): void { const root = this._internalRoot; - const work = new ReactWork(); callback = callback === undefined ? null : callback; if (__DEV__) { warnOnInvalidCallback(callback, 'render'); } - if (callback !== null) { - work.then(callback); - } - updateContainer(children, root, null, work._onCommit); - return work; + updateContainer(children, root, null, callback); }; ReactRoot.prototype.unmount = ReactSyncRoot.prototype.unmount = function( callback: ?() => mixed, -): Work { +): void { const root = this._internalRoot; - const work = new ReactWork(); callback = callback === undefined ? null : callback; if (__DEV__) { warnOnInvalidCallback(callback, 'render'); } - if (callback !== null) { - work.then(callback); - } - updateContainer(null, root, null, work._onCommit); - return work; -}; - -// Sync roots cannot create batches. Only concurrent ones. -ReactRoot.prototype.createBatch = function(): Batch { - const batch = new ReactBatch(this); - const expirationTime = batch._expirationTime; - - const internalRoot = this._internalRoot; - const firstBatch = internalRoot.firstBatch; - if (firstBatch === null) { - internalRoot.firstBatch = batch; - batch._next = null; - } else { - // Insert sorted by expiration time then insertion order - let insertAfter = null; - let insertBefore = firstBatch; - while ( - insertBefore !== null && - insertBefore._expirationTime >= expirationTime - ) { - insertAfter = insertBefore; - insertBefore = insertBefore._next; - } - batch._next = insertBefore; - if (insertAfter !== null) { - insertAfter._next = batch; - } - } - - return batch; + updateContainer(null, root, null, callback); }; /** @@ -529,7 +295,7 @@ let warnedAboutHydrateAPI = false; function legacyCreateRootFromDOMContainer( container: DOMContainer, forceHydrate: boolean, -): _ReactSyncRoot { +): _ReactRoot { const shouldHydrate = forceHydrate || shouldHydrateDueToLegacyHeuristic(container); // First clear any existing content. @@ -593,7 +359,7 @@ function legacyRenderSubtreeIntoContainer( // TODO: Without `any` type, Flow says "Property cannot be accessed on any // member of intersection type." Whyyyyyy. - let root: _ReactSyncRoot = (container._reactRootContainer: any); + let root: _ReactRoot = (container._reactRootContainer: any); let fiberRoot; if (!root) { // Initial mount @@ -899,7 +665,7 @@ function createRoot( function createSyncRoot( container: DOMContainer, options?: RootOptions, -): _ReactSyncRoot { +): _ReactRoot { const functionName = enableStableConcurrentModeAPIs ? 'createRoot' : 'unstable_createRoot'; diff --git a/packages/react-dom/src/events/__tests__/DOMEventResponderSystem-test.internal.js b/packages/react-dom/src/events/__tests__/DOMEventResponderSystem-test.internal.js index 65e01fff98c2..bb442d81d9d1 100644 --- a/packages/react-dom/src/events/__tests__/DOMEventResponderSystem-test.internal.js +++ b/packages/react-dom/src/events/__tests__/DOMEventResponderSystem-test.internal.js @@ -857,7 +857,7 @@ describe('DOMEventResponderSystem', () => { function Test({counter}) { const listener = React.unstable_useResponder(TestResponder, {counter}); - + Scheduler.unstable_yieldValue('Test'); return (