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.

Sign up for GitHub

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

Jump to bottom

Remove blocking mode and blocking root #20888

Merged
merged 5 commits into from
Feb 28, 2021
Merged

Remove blocking mode and blocking root #20888

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b688b1a
Remove blocking mode and blocking root
rickhanlonii Feb 26, 2021
2fc60bf
Add back SuspenseList test
rickhanlonii Feb 26, 2021
e17afb6
Clean up ReactDOMLegacyRoot
rickhanlonii Feb 26, 2021
a263481
Remove dupe ConcurrentRoot
rickhanlonii Feb 26, 2021
dfb223e
Update comment
rickhanlonii Feb 26, 2021
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
2 changes: 0 additions & 2 deletions packages/react-dom/index.classic.fb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ export {
unmountComponentAtNode,
createRoot,
createRoot as unstable_createRoot,
createBlockingRoot,
createBlockingRoot as unstable_createBlockingRoot,
unstable_flushControlled,
unstable_scheduleHydration,
unstable_runWithPriority,
Expand Down
1 change: 0 additions & 1 deletion packages/react-dom/index.experimental.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ export {
unmountComponentAtNode,
// exposeConcurrentModeAPIs
createRoot as unstable_createRoot,
createBlockingRoot as unstable_createBlockingRoot,
unstable_flushControlled,
unstable_scheduleHydration,
// DO NOT USE: Temporarily exposing this to migrate off of Scheduler.runWithPriority.
Expand Down
2 changes: 0 additions & 2 deletions packages/react-dom/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@ export {
unmountComponentAtNode,
createRoot,
createRoot as unstable_createRoot,
createBlockingRoot,
createBlockingRoot as unstable_createBlockingRoot,
unstable_flushControlled,
unstable_scheduleHydration,
unstable_runWithPriority,
Expand Down
2 changes: 0 additions & 2 deletions packages/react-dom/index.modern.fb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ export {
version,
createRoot,
createRoot as unstable_createRoot,
createBlockingRoot,
createBlockingRoot as unstable_createBlockingRoot,
unstable_flushControlled,
unstable_scheduleHydration,
unstable_runWithPriority,
Expand Down
27 changes: 0 additions & 27 deletions packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,33 +593,6 @@ describe('ReactDOMFiberAsync', () => {
expect(containerC.textContent).toEqual('Finished');
});

describe('createBlockingRoot', () => {
// @gate experimental
it('updates flush without yielding in the next event', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is an equivalent test useful for CM now?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so because this is blocking specific behavior.

There are 10 tests above for concurrent mode behavior:
Screen Shot 2021-02-26 at 10 25 22 AM

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes but doesn't CM "update without yielding" for non-startTransition "default" events too, now? Outside of FB. If yes, do we have a test for this?

It's not quite blocking-specific behavior if we're saying CM inherits some features of blocking mode now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet, we still need to make the changes for default updates to flush sync after a microtask. I'll be sure to add a test for that when we do.

const root = ReactDOM.unstable_createBlockingRoot(container);

function Text(props) {
Scheduler.unstable_yieldValue(props.text);
return props.text;
}

root.render(
<>
<Text text="A" />
<Text text="B" />
<Text text="C" />
</>,
);

// Nothing should have rendered yet
expect(container.textContent).toEqual('');

// Everything should render immediately in the next event
expect(Scheduler).toFlushExpired(['A', 'B', 'C']);
expect(container.textContent).toEqual('ABC');
});
});

// @gate experimental
it('unmounted roots should never clear newer root content from a container', () => {
const ref = React.createRef();
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ describe('ReactDOMServerPartialHydration', () => {
}).toErrorDev(
'Warning: Cannot hydrate Suspense in legacy mode. Switch from ' +
'ReactDOM.hydrate(element, container) to ' +
'ReactDOM.createBlockingRoot(container, { hydrate: true })' +
'ReactDOM.createRoot(container, { hydrate: true })' +
'.render(element) or remove the Suspense components from the server ' +
'rendered components.' +
'\n in Suspense (at **)' +
Expand Down
35 changes: 0 additions & 35 deletions packages/react-dom/src/__tests__/ReactDOMServerSuspense-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;
let act;

function initModules() {
// Reset warning cache.
Expand All @@ -25,7 +24,6 @@ function initModules() {
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');
act = ReactTestUtils.unstable_concurrentAct;

// Make them available to the helpers.
return {
Expand Down Expand Up @@ -105,39 +103,6 @@ describe('ReactDOMServerSuspense', () => {
);
});

// @gate experimental
it('server renders a SuspenseList component and its children', async () => {
const example = (
<React.SuspenseList>
<React.Suspense fallback="Loading A">
<div>A</div>
</React.Suspense>
<React.Suspense fallback="Loading B">
<div>B</div>
</React.Suspense>
</React.SuspenseList>
);
const element = await serverRender(example);
const parent = element.parentNode;
const divA = parent.children[0];
expect(divA.tagName).toBe('DIV');
expect(divA.textContent).toBe('A');
const divB = parent.children[1];
expect(divB.tagName).toBe('DIV');
expect(divB.textContent).toBe('B');

act(() => {
const root = ReactDOM.createBlockingRoot(parent, {hydrate: true});
rickhanlonii marked this conversation as resolved.
Show resolved Hide resolved
root.render(example);
});

const parent2 = element.parentNode;
const divA2 = parent2.children[0];
const divB2 = parent2.children[1];
expect(divA).toBe(divA2);
expect(divB).toBe(divB2);
});

// TODO: Remove this in favor of @gate pragma
if (__EXPERIMENTAL__) {
itThrowsWhenRendering(
Expand Down
58 changes: 6 additions & 52 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,33 +72,6 @@ describe('ReactTestUtils.act()', () => {

runActTests('legacy mode', renderLegacy, unmountLegacy, rerenderLegacy);

// and then in blocking mode
if (__EXPERIMENTAL__) {
let blockingRoot = null;
const renderBatched = (el, dom) => {
blockingRoot = ReactDOM.unstable_createBlockingRoot(dom);
blockingRoot.render(el);
};

const unmountBatched = dom => {
if (blockingRoot !== null) {
blockingRoot.unmount();
blockingRoot = null;
}
};

const rerenderBatched = el => {
blockingRoot.render(el);
};

runActTests(
'blocking mode',
renderBatched,
unmountBatched,
rerenderBatched,
);
}

describe('unacted effects', () => {
function App() {
React.useEffect(() => {}, []);
Expand All @@ -124,19 +97,6 @@ describe('ReactTestUtils.act()', () => {
]);
});

// @gate experimental
it('warns in blocking mode', () => {
expect(() => {
const root = ReactDOM.unstable_createBlockingRoot(
document.createElement('div'),
);
root.render(<App />);
Scheduler.unstable_flushAll();
}).toErrorDev([
'An update to App ran an effect, but was not wrapped in act(...)',
]);
});

// @gate experimental
it('warns in concurrent mode', () => {
expect(() => {
Expand Down Expand Up @@ -731,14 +691,10 @@ function runActTests(label, render, unmount, rerender) {

it('triggers fallbacks if available', async () => {
if (label !== 'legacy mode') {
// FIXME: Support for Blocking* and Concurrent Mode were
// intentionally removed from the public version of `act`. It will
// be added back in a future major version, before Blocking and and
// Concurrent Mode are officially released. Consider disabling all
// non-Legacy tests in this suite until then.
//
// *Blocking Mode actually does happen to work, though
// not "officially" since it's an unreleased feature.
// FIXME: Support for Concurrent Root intentionally removed
// from the public version of `act`. It will be added back in
// a future major version, Concurrent Root officially released.
rickhanlonii marked this conversation as resolved.
Show resolved Hide resolved
// Consider skipping all non-Legacy tests in this suite until then.
return;
}

Expand Down Expand Up @@ -794,10 +750,8 @@ function runActTests(label, render, unmount, rerender) {
// In Concurrent Mode, refresh transitions delay indefinitely.
expect(document.querySelector('[data-test-id=spinner]')).toBeNull();
} else {
// In Legacy Mode and Blocking Mode, all fallbacks are forced to
// display, even during a refresh transition.
// TODO: Consider delaying indefinitely in Blocking Mode, to match
// Concurrent Mode semantics.
// In Legacy Mode, all fallbacks are forced to display,
// even during a refresh transition.
expect(
document.querySelector('[data-test-id=spinner]'),
).not.toBeNull();
Expand Down
19 changes: 0 additions & 19 deletions packages/react-dom/src/__tests__/ReactUnmockedSchedulerWarning-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,22 +43,3 @@ it('should warn when rendering in concurrent mode', () => {
ReactDOM.unstable_createRoot(document.createElement('div')).render(<App />);
}).toErrorDev([]);
});

// @gate experimental
it('should warn when rendering in blocking mode', () => {
expect(() => {
ReactDOM.unstable_createBlockingRoot(document.createElement('div')).render(
<App />,
);
}).toErrorDev(
'In Concurrent or Sync modes, the "scheduler" module needs to be mocked ' +
'to guarantee consistent behaviour across tests and browsers.',
{withoutStack: true},
);
// does not warn twice
expect(() => {
ReactDOM.unstable_createBlockingRoot(document.createElement('div')).render(
<App />,
);
}).toErrorDev([]);
});
3 changes: 1 addition & 2 deletions packages/react-dom/src/client/ReactDOM.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
unstable_renderSubtreeIntoContainer,
unmountComponentAtNode,
} from './ReactDOMLegacy';
import {createRoot, createBlockingRoot, isValidContainer} from './ReactDOMRoot';
import {createRoot, isValidContainer} from './ReactDOMRoot';
import {createEventHandle} from './ReactDOMEventHandle';

import {
Expand Down Expand Up @@ -201,7 +201,6 @@ export {
unmountComponentAtNode,
// exposeConcurrentModeAPIs
createRoot,
createBlockingRoot,
flushControlled as unstable_flushControlled,
scheduleHydration as unstable_scheduleHydration,
// Disabled behind disableUnstableRenderSubtreeIntoContainer
Expand Down
28 changes: 6 additions & 22 deletions packages/react-dom/src/client/ReactDOMRoot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,25 +51,21 @@ import {
registerMutableSourceForHydration,
} from 'react-reconciler/src/ReactFiberReconciler';
import invariant from 'shared/invariant';
import {
BlockingRoot,
ConcurrentRoot,
LegacyRoot,
} from 'react-reconciler/src/ReactRootTags';
import {ConcurrentRoot, LegacyRoot} from 'react-reconciler/src/ReactRootTags';

function ReactDOMRoot(container: Container, options: void | RootOptions) {
this._internalRoot = createRootImpl(container, ConcurrentRoot, options);
}

function ReactDOMBlockingRoot(
function ReactDOMLegacyRoot(
container: Container,
tag: RootTag,
rickhanlonii marked this conversation as resolved.
Show resolved Hide resolved
options: void | RootOptions,
) {
this._internalRoot = createRootImpl(container, tag, options);
this._internalRoot = createRootImpl(container, LegacyRoot, options);
}

ReactDOMRoot.prototype.render = ReactDOMBlockingRoot.prototype.render = function(
ReactDOMRoot.prototype.render = ReactDOMLegacyRoot.prototype.render = function(
children: ReactNodeList,
): void {
const root = this._internalRoot;
Expand Down Expand Up @@ -99,7 +95,7 @@ ReactDOMRoot.prototype.render = ReactDOMBlockingRoot.prototype.render = function
updateContainer(children, root, null, null);
};

ReactDOMRoot.prototype.unmount = ReactDOMBlockingRoot.prototype.unmount = function(): void {
ReactDOMRoot.prototype.unmount = ReactDOMLegacyRoot.prototype.unmount = function(): void {
if (__DEV__) {
if (typeof arguments[0] === 'function') {
console.error(
Expand Down Expand Up @@ -169,23 +165,11 @@ export function createRoot(
return new ReactDOMRoot(container, options);
}

export function createBlockingRoot(
container: Container,
options?: RootOptions,
): RootType {
invariant(
isValidContainer(container),
'createRoot(...): Target container is not a DOM element.',
);
warnIfReactDOMContainerInDEV(container);
return new ReactDOMBlockingRoot(container, BlockingRoot, options);
}

export function createLegacyRoot(
container: Container,
options?: RootOptions,
): RootType {
return new ReactDOMBlockingRoot(container, LegacyRoot, options);
return new ReactDOMLegacyRoot(container, LegacyRoot, options);
rickhanlonii marked this conversation as resolved.
Show resolved Hide resolved
}

export function isValidContainer(node: mixed): boolean {
Expand Down
1 change: 0 additions & 1 deletion packages/react-noop-renderer/src/ReactNoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ export const {
getPendingChildren,
getOrCreateRootContainer,
createRoot,
createBlockingRoot,
createLegacyRoot,
getChildrenAsJSX,
getPendingChildrenAsJSX,
Expand Down
1 change: 0 additions & 1 deletion packages/react-noop-renderer/src/ReactNoopPersistent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ export const {
getPendingChildren,
getOrCreateRootContainer,
createRoot,
createBlockingRoot,
createLegacyRoot,
getChildrenAsJSX,
getPendingChildrenAsJSX,
Expand Down
33 changes: 1 addition & 32 deletions packages/react-noop-renderer/src/createReactNoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,7 @@ import type {RootTag} from 'react-reconciler/src/ReactRootTags';

import * as Scheduler from 'scheduler/unstable_mock';
import {REACT_FRAGMENT_TYPE, REACT_ELEMENT_TYPE} from 'shared/ReactSymbols';
import {
ConcurrentRoot,
BlockingRoot,
LegacyRoot,
} from 'react-reconciler/src/ReactRootTags';
import {ConcurrentRoot, LegacyRoot} from 'react-reconciler/src/ReactRootTags';

import {
enableNativeEventPriorityInference,
Expand Down Expand Up @@ -756,33 +752,6 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
};
},

createBlockingRoot() {
const container = {
rootID: '' + idCounter++,
pendingChildren: [],
children: [],
};
const fiberRoot = NoopRenderer.createContainer(
container,
BlockingRoot,
false,
null,
null,
);
return {
_Scheduler: Scheduler,
render(children: ReactNodeList) {
NoopRenderer.updateContainer(children, fiberRoot, null, null);
},
getChildren() {
return getChildren(container);
},
getChildrenAsJSX() {
return getChildrenAsJSX(container);
},
};
},

createLegacyRoot() {
const container = {
rootID: '' + idCounter++,
Expand Down
Loading