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

[Test] Update flushSync tests to use react-dom #28490

Merged
merged 1 commit into from
Mar 5, 2024
Merged

[Test] Update flushSync tests to use react-dom #28490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
148 changes: 102 additions & 46 deletions packages/react-reconciler/src/__tests__/ReactFlushSync-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
let React;
let ReactNoop;
let ReactDOM;
let ReactDOMClient;
let Scheduler;
let act;
let useState;
Expand All @@ -15,7 +16,8 @@ describe('ReactFlushSync', () => {
jest.resetModules();

React = require('react');
ReactNoop = require('react-noop-renderer');
ReactDOM = require('react-dom');
ReactDOMClient = require('react-dom/client');
Scheduler = require('scheduler');
act = require('internal-test-utils').act;
useState = React.useState;
Expand All @@ -32,7 +34,49 @@ describe('ReactFlushSync', () => {
return text;
}

test('changes priority of updates in useEffect', async () => {
function getVisibleChildren(element: Element): React$Node {
const children = [];
let node: any = element.firstChild;
while (node) {
if (node.nodeType === 1) {
if (
((node.tagName !== 'SCRIPT' && node.tagName !== 'script') ||
node.hasAttribute('data-meaningful')) &&
node.tagName !== 'TEMPLATE' &&
node.tagName !== 'template' &&
!node.hasAttribute('hidden') &&
!node.hasAttribute('aria-hidden')
) {
const props: any = {};
const attributes = node.attributes;
for (let i = 0; i < attributes.length; i++) {
if (
attributes[i].name === 'id' &&
attributes[i].value.includes(':')
) {
// We assume this is a React added ID that's a non-visual implementation detail.
continue;
}
props[attributes[i].name] = attributes[i].value;
}
props.children = getVisibleChildren(node);
children.push(
require('react').createElement(node.tagName.toLowerCase(), props),
);
}
} else if (node.nodeType === 3) {
children.push(node.data);
}
node = node.nextSibling;
}
return children.length === 0
? undefined
: children.length === 1
? children[0]
: children;
}

it('changes priority of updates in useEffect', async () => {
spyOnDev(console, 'error').mockImplementation(() => {});

function App() {
Expand All @@ -41,13 +85,14 @@ describe('ReactFlushSync', () => {
useEffect(() => {
if (syncState !== 1) {
setState(1);
ReactNoop.flushSync(() => setSyncState(1));
ReactDOM.flushSync(() => setSyncState(1));
}
}, [syncState, state]);
return <Text text={`${syncState}, ${state}`} />;
}

const root = ReactNoop.createRoot();
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
await act(async () => {
React.startTransition(() => {
root.render(<App />);
Expand All @@ -62,7 +107,7 @@ describe('ReactFlushSync', () => {
);

// The remaining update is not sync
ReactNoop.flushSync();
ReactDOM.flushSync();
assertLog([]);

if (gate(flags => flags.enableUnifiedSyncLane)) {
Expand All @@ -72,7 +117,7 @@ describe('ReactFlushSync', () => {
await waitForPaint(['1, 1']);
}
});
expect(root).toMatchRenderedOutput('1, 1');
expect(getVisibleChildren(container)).toEqual('1, 1');
Copy link
Member

@rickhanlonii rickhanlonii Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What do you think about creating a matcher for this? Seems common and we're starting to copy this helper over to multiple places. It would be nice to standardize the ReactDOM tests asserting container content on one patter because it's all over the place right now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it's generally very useful however there are some special cases in ReactDOMFloat tests where we do even more special casing because we need to assert certain things like scripts which the basic version of this ignores. Maybe instead of a matcher we just have a test util for this function so it's a little clearer what's happening when you choose to specialize the implementation. Or maybe the float one can be made the default one


if (__DEV__) {
expect(console.error.mock.calls[0][0]).toContain(
Expand All @@ -83,7 +128,7 @@ describe('ReactFlushSync', () => {
}
});

test('nested with startTransition', async () => {
it('supports nested flushSync with startTransition', async () => {
let setSyncState;
let setState;
function App() {
Expand All @@ -94,20 +139,21 @@ describe('ReactFlushSync', () => {
return <Text text={`${syncState}, ${state}`} />;
}

const root = ReactNoop.createRoot();
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
await act(() => {
root.render(<App />);
});
assertLog(['0, 0']);
expect(root).toMatchRenderedOutput('0, 0');
expect(getVisibleChildren(container)).toEqual('0, 0');

await act(() => {
ReactNoop.flushSync(() => {
ReactDOM.flushSync(() => {
startTransition(() => {
// This should be async even though flushSync is on the stack, because
// startTransition is closer.
setState(1);
ReactNoop.flushSync(() => {
ReactDOM.flushSync(() => {
// This should be async even though startTransition is on the stack,
// because flushSync is closer.
setSyncState(1);
Expand All @@ -116,24 +162,25 @@ describe('ReactFlushSync', () => {
});
// Only the sync update should have flushed
assertLog(['1, 0']);
expect(root).toMatchRenderedOutput('1, 0');
expect(getVisibleChildren(container)).toEqual('1, 0');
});
// Now the async update has flushed, too.
assertLog(['1, 1']);
expect(root).toMatchRenderedOutput('1, 1');
expect(getVisibleChildren(container)).toEqual('1, 1');
});

test('flushes passive effects synchronously when they are the result of a sync render', async () => {
it('flushes passive effects synchronously when they are the result of a sync render', async () => {
function App() {
useEffect(() => {
Scheduler.log('Effect');
}, []);
return <Text text="Child" />;
}

const root = ReactNoop.createRoot();
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
await act(() => {
ReactNoop.flushSync(() => {
ReactDOM.flushSync(() => {
root.render(<App />);
});
assertLog([
Expand All @@ -142,35 +189,37 @@ describe('ReactFlushSync', () => {
// flushSync should flush it.
'Effect',
]);
expect(root).toMatchRenderedOutput('Child');
expect(getVisibleChildren(container)).toEqual('Child');
});
});

test('do not flush passive effects synchronously after render in legacy mode', async () => {
// @gate !disableLegacyMode
Copy link
Member

Choose a reason for hiding this comment

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

woah SICK YES

it('does not flush passive effects synchronously after render in legacy mode', async () => {
function App() {
useEffect(() => {
Scheduler.log('Effect');
}, []);
return <Text text="Child" />;
}

const root = ReactNoop.createLegacyRoot();
const container = document.createElement('div');
await act(() => {
ReactNoop.flushSync(() => {
root.render(<App />);
ReactDOM.flushSync(() => {
ReactDOM.render(<App />, container);
});
assertLog([
'Child',
// Because we're in legacy mode, we shouldn't have flushed the passive
// effects yet.
]);
expect(root).toMatchRenderedOutput('Child');
expect(getVisibleChildren(container)).toEqual('Child');
});
// Effect flushes after paint.
assertLog(['Effect']);
});

test('flush pending passive effects before scope is called in legacy mode', async () => {
// @gate !disableLegacyMode
it('flushes pending passive effects before scope is called in legacy mode', async () => {
let currentStep = 0;

function App({step}) {
Expand All @@ -181,82 +230,89 @@ describe('ReactFlushSync', () => {
return <Text text={step} />;
}

const root = ReactNoop.createLegacyRoot();
const container = document.createElement('div');
await act(() => {
ReactNoop.flushSync(() => {
root.render(<App step={1} />);
ReactDOM.flushSync(() => {
ReactDOM.render(<App step={1} />, container);
});
assertLog([
1,
// Because we're in legacy mode, we shouldn't have flushed the passive
// effects yet.
]);
expect(root).toMatchRenderedOutput('1');
expect(getVisibleChildren(container)).toEqual('1');

ReactNoop.flushSync(() => {
ReactDOM.flushSync(() => {
// This should render step 2 because the passive effect has already
// fired, before the scope function is called.
root.render(<App step={currentStep + 1} />);
ReactDOM.render(<App step={currentStep + 1} />, container);
});
assertLog(['Effect: 1', 2]);
expect(root).toMatchRenderedOutput('2');
expect(getVisibleChildren(container)).toEqual('2');
});
assertLog(['Effect: 2']);
});

test("do not flush passive effects synchronously when they aren't the result of a sync render", async () => {
it("does not flush passive effects synchronously when they aren't the result of a sync render", async () => {
function App() {
useEffect(() => {
Scheduler.log('Effect');
}, []);
return <Text text="Child" />;
}

const root = ReactNoop.createRoot();
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
await act(async () => {
root.render(<App />);
await waitForPaint([
'Child',
// Because the passive effect was not the result of a sync update, it
// should not flush before paint.
]);
expect(root).toMatchRenderedOutput('Child');
expect(getVisibleChildren(container)).toEqual('Child');
});
// Effect flushes after paint.
assertLog(['Effect']);
});

test('does not flush pending passive effects', async () => {
it('does not flush pending passive effects', async () => {
function App() {
useEffect(() => {
Scheduler.log('Effect');
}, []);
return <Text text="Child" />;
}

const root = ReactNoop.createRoot();
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
await act(async () => {
root.render(<App />);
await waitForPaint(['Child']);
expect(root).toMatchRenderedOutput('Child');
expect(getVisibleChildren(container)).toEqual('Child');

// Passive effects are pending. Calling flushSync should not affect them.
ReactNoop.flushSync();
ReactDOM.flushSync();
// Effects still haven't fired.
assertLog([]);
});
// Now the effects have fired.
assertLog(['Effect']);
});

test('completely exhausts synchronous work queue even if something throws', async () => {
it('completely exhausts synchronous work queue even if something throws', async () => {
function Throws({error}) {
throw error;
}

const root1 = ReactNoop.createRoot();
const root2 = ReactNoop.createRoot();
const root3 = ReactNoop.createRoot();
const container1 = document.createElement('div');
const root1 = ReactDOMClient.createRoot(container1);

const container2 = document.createElement('div');
const root2 = ReactDOMClient.createRoot(container2);

const container3 = document.createElement('div');
const root3 = ReactDOMClient.createRoot(container3);

await act(async () => {
root1.render(<Text text="Hi" />);
Expand All @@ -270,7 +326,7 @@ describe('ReactFlushSync', () => {

let error;
try {
ReactNoop.flushSync(() => {
ReactDOM.flushSync(() => {
root1.render(<Throws error={aahh} />);
root2.render(<Throws error={nooo} />);
root3.render(<Text text="aww" />);
Expand All @@ -283,9 +339,9 @@ describe('ReactFlushSync', () => {
// earlier updates errored.
assertLog(['aww']);
// Roots 1 and 2 were unmounted.
expect(root1).toMatchRenderedOutput(null);
expect(root2).toMatchRenderedOutput(null);
expect(root3).toMatchRenderedOutput('aww');
expect(getVisibleChildren(container1)).toEqual(undefined);
expect(getVisibleChildren(container2)).toEqual(undefined);
expect(getVisibleChildren(container3)).toEqual('aww');

// Because there were multiple errors, React threw an AggregateError.
// eslint-disable-next-line no-undef
Expand Down
Loading