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

Allow async blocks in to(Error|Warn)Dev #25338

Merged
merged 4 commits into from
Dec 1, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -1777,7 +1777,7 @@ describe('ReactHooksWithNoopRenderer', () => {
}, [props.count]);
return <Text text={'Count: ' + count} />;
}
expect(() =>
expect(() => {
act(() => {
ReactNoop.render(<Counter count={0} />, () =>
Scheduler.unstable_yieldValue('Sync effect'),
Expand All @@ -1787,8 +1787,8 @@ describe('ReactHooksWithNoopRenderer', () => {
'Sync effect',
]);
expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]);
}),
).toErrorDev('flushSync was called from inside a lifecycle method');
});
}).toErrorDev('flushSync was called from inside a lifecycle method');
expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]);
});

Expand Down Expand Up @@ -2648,32 +2648,32 @@ describe('ReactHooksWithNoopRenderer', () => {
}

const root1 = ReactNoop.createRoot();
expect(() =>
expect(() => {
act(() => {
root1.render(<App return={17} />);
}),
).toErrorDev([
});
}).toErrorDev([
'Warning: useEffect must not return anything besides a ' +
'function, which is used for clean-up. You returned: 17',
]);

const root2 = ReactNoop.createRoot();
expect(() =>
expect(() => {
act(() => {
root2.render(<App return={null} />);
}),
).toErrorDev([
});
}).toErrorDev([
'Warning: useEffect must not return anything besides a ' +
'function, which is used for clean-up. You returned null. If your ' +
'effect does not require clean up, return undefined (or nothing).',
]);

const root3 = ReactNoop.createRoot();
expect(() =>
expect(() => {
act(() => {
root3.render(<App return={Promise.resolve()} />);
}),
).toErrorDev([
});
}).toErrorDev([
'Warning: useEffect must not return anything besides a ' +
'function, which is used for clean-up.\n\n' +
'It looks like you wrote useEffect(async () => ...) or returned a Promise.',
Expand Down Expand Up @@ -3052,32 +3052,32 @@ describe('ReactHooksWithNoopRenderer', () => {
}

const root1 = ReactNoop.createRoot();
expect(() =>
expect(() => {
act(() => {
root1.render(<App return={17} />);
}),
).toErrorDev([
});
}).toErrorDev([
'Warning: useInsertionEffect must not return anything besides a ' +
'function, which is used for clean-up. You returned: 17',
]);

const root2 = ReactNoop.createRoot();
expect(() =>
expect(() => {
act(() => {
root2.render(<App return={null} />);
}),
).toErrorDev([
});
}).toErrorDev([
'Warning: useInsertionEffect must not return anything besides a ' +
'function, which is used for clean-up. You returned null. If your ' +
'effect does not require clean up, return undefined (or nothing).',
]);

const root3 = ReactNoop.createRoot();
expect(() =>
expect(() => {
act(() => {
root3.render(<App return={Promise.resolve()} />);
}),
).toErrorDev([
});
}).toErrorDev([
'Warning: useInsertionEffect must not return anything besides a ' +
'function, which is used for clean-up.\n\n' +
'It looks like you wrote useInsertionEffect(async () => ...) or returned a Promise.',
Expand All @@ -3104,11 +3104,11 @@ describe('ReactHooksWithNoopRenderer', () => {
}

const root = ReactNoop.createRoot();
expect(() =>
expect(() => {
act(() => {
root.render(<App />);
}),
).toErrorDev(['Warning: useInsertionEffect must not schedule updates.']);
});
}).toErrorDev(['Warning: useInsertionEffect must not schedule updates.']);

expect(() => {
act(() => {
Expand Down Expand Up @@ -3359,32 +3359,32 @@ describe('ReactHooksWithNoopRenderer', () => {
}

const root1 = ReactNoop.createRoot();
expect(() =>
expect(() => {
act(() => {
root1.render(<App return={17} />);
}),
).toErrorDev([
});
}).toErrorDev([
'Warning: useLayoutEffect must not return anything besides a ' +
'function, which is used for clean-up. You returned: 17',
]);

const root2 = ReactNoop.createRoot();
expect(() =>
expect(() => {
act(() => {
root2.render(<App return={null} />);
}),
).toErrorDev([
});
}).toErrorDev([
'Warning: useLayoutEffect must not return anything besides a ' +
'function, which is used for clean-up. You returned null. If your ' +
'effect does not require clean up, return undefined (or nothing).',
]);

const root3 = ReactNoop.createRoot();
expect(() =>
expect(() => {
act(() => {
root3.render(<App return={Promise.resolve()} />);
}),
).toErrorDev([
});
}).toErrorDev([
'Warning: useLayoutEffect must not return anything besides a ' +
'function, which is used for clean-up.\n\n' +
'It looks like you wrote useLayoutEffect(async () => ...) or returned a Promise.',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ describe 'ReactCoffeeScriptClass', ->
expect(->
act ->
root.render React.createElement(Foo, foo: 'foo')
return
).toErrorDev 'Foo: getDerivedStateFromProps() is defined as an instance method and will be ignored. Instead, declare it as a static method.'

it 'warns if getDerivedStateFromError is not static', ->
Expand All @@ -144,6 +145,7 @@ describe 'ReactCoffeeScriptClass', ->
expect(->
act ->
root.render React.createElement(Foo, foo: 'foo')
return
).toErrorDev 'Foo: getDerivedStateFromError() is defined as an instance method and will be ignored. Instead, declare it as a static method.'

it 'warns if getSnapshotBeforeUpdate is static', ->
Expand All @@ -155,6 +157,7 @@ describe 'ReactCoffeeScriptClass', ->
expect(->
act ->
root.render React.createElement(Foo, foo: 'foo')
return
).toErrorDev 'Foo: getSnapshotBeforeUpdate() is defined as a static method and will be ignored. Instead, declare it as an instance method.'

it 'warns if state not initialized before static getDerivedStateFromProps', ->
Expand All @@ -171,6 +174,7 @@ describe 'ReactCoffeeScriptClass', ->
expect(->
act ->
root.render React.createElement(Foo, foo: 'foo')
return
).toErrorDev (
'`Foo` uses `getDerivedStateFromProps` but its initial state is ' +
'undefined. This is not recommended. Instead, define the initial state by ' +
Expand Down
16 changes: 12 additions & 4 deletions packages/react/src/__tests__/ReactES6Class-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,9 @@ describe('ReactES6Class', () => {
return <div />;
}
}
expect(() => act(() => root.render(<Foo foo="foo" />))).toErrorDev(
expect(() => {
act(() => root.render(<Foo foo="foo" />));
}).toErrorDev(
'Foo: getDerivedStateFromProps() is defined as an instance method ' +
'and will be ignored. Instead, declare it as a static method.',
);
Expand All @@ -164,7 +166,9 @@ describe('ReactES6Class', () => {
return <div />;
}
}
expect(() => act(() => root.render(<Foo foo="foo" />))).toErrorDev(
expect(() => {
act(() => root.render(<Foo foo="foo" />));
}).toErrorDev(
'Foo: getDerivedStateFromError() is defined as an instance method ' +
'and will be ignored. Instead, declare it as a static method.',
);
Expand All @@ -177,7 +181,9 @@ describe('ReactES6Class', () => {
return <div />;
}
}
expect(() => act(() => root.render(<Foo foo="foo" />))).toErrorDev(
expect(() => {
act(() => root.render(<Foo foo="foo" />));
}).toErrorDev(
'Foo: getSnapshotBeforeUpdate() is defined as a static method ' +
'and will be ignored. Instead, declare it as an instance method.',
);
Expand All @@ -195,7 +201,9 @@ describe('ReactES6Class', () => {
return <div className={`${this.state.foo} ${this.state.bar}`} />;
}
}
expect(() => act(() => root.render(<Foo foo="foo" />))).toErrorDev(
expect(() => {
act(() => root.render(<Foo foo="foo" />));
}).toErrorDev(
'`Foo` uses `getDerivedStateFromProps` but its initial state is ' +
'undefined. This is not recommended. Instead, define the initial state by ' +
'assigning an object to `this.state` in the constructor of `Foo`. ' +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ let useSyncExternalStoreWithSelector;
let React;
let ReactDOM;
let ReactDOMClient;
let ReactFeatureFlags;
let Scheduler;
let act;
let useState;
Expand Down Expand Up @@ -48,6 +49,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
React = require('react');
ReactDOM = require('react-dom');
ReactDOMClient = require('react-dom/client');
ReactFeatureFlags = require('shared/ReactFeatureFlags');
Scheduler = require('scheduler');
useState = React.useState;
useEffect = React.useEffect;
Expand Down Expand Up @@ -882,8 +884,7 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {

describe('selector and isEqual error handling in extra', () => {
let ErrorBoundary;
beforeAll(() => {
spyOnDev(console, 'warn');
beforeEach(() => {
ErrorBoundary = class extends React.Component {
state = {error: null};
static getDerivedStateFromError(error) {
Expand Down Expand Up @@ -929,9 +930,15 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {

expect(container.textContent).toEqual('A');

await act(() => {
store.set({});
});
await expect(async () => {
await act(async () => {
store.set({});
});
}).toWarnDev(
ReactFeatureFlags.enableUseRefAccessWarning
? ['Warning: App: Unsafe read of a mutable value during render.']
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Intended or should this be a TODO?

: [],
);
expect(container.textContent).toEqual('Malformed state');
});

Expand Down Expand Up @@ -968,9 +975,15 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {

expect(container.textContent).toEqual('A');

await act(() => {
store.set({});
});
await expect(async () => {
await act(() => {
store.set({});
});
}).toWarnDev(
ReactFeatureFlags.enableUseRefAccessWarning
? ['Warning: App: Unsafe read of a mutable value during render.']
: [],
);
expect(container.textContent).toEqual('Malformed state');
});
});
Expand Down
50 changes: 43 additions & 7 deletions scripts/jest/matchers/toWarnDev.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,7 @@ const createMatcherFor = (consoleMethod, matcherName) =>
// Avoid using Jest's built-in spy since it can't be removed.
console[consoleMethod] = consoleSpy;

try {
callback();
} catch (error) {
caughtError = error;
} finally {
const onFinally = () => {
// Restore the unspied method so that unexpected errors fail tests.
console[consoleMethod] = originalMethod;

Expand Down Expand Up @@ -259,12 +255,52 @@ const createMatcherFor = (consoleMethod, matcherName) =>
}

return {pass: true};
};

let returnPromise = null;
try {
const result = callback();

if (
typeof result === 'object' &&
result !== null &&
typeof result.then === 'function'
) {
// `act` returns a thenable that can't be chained.
// Once `act(async () => {}).then(() => {}).then(() => {})` works
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should act() be chainable?

// we can just return `result.then(onFinally, error => ...)`
returnPromise = new Promise((resolve, reject) => {
result.then(
() => {
resolve(onFinally());
},
error => {
caughtError = error;
return resolve(onFinally());
}
);
});
}
} catch (error) {
caughtError = error;
} finally {
return returnPromise === null ? onFinally() : returnPromise;
}
} else {
// Any uncaught errors or warnings should fail tests in production mode.
callback();
const result = callback();

return {pass: true};
if (
typeof result === 'object' &&
result !== null &&
typeof result.then === 'function'
) {
return result.then(() => {
return {pass: true};
});
} else {
return {pass: true};
}
}
};

Expand Down