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

[fail] Only warn on unacted effects for strict / non sync modes #16041

Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -53,32 +53,23 @@ describe('ReactDOMHooks', () => {
return 3 * n;
}

// we explicitly catch the missing act() warnings
// to simulate this tricky repro
expect(() => {
ReactDOM.render(<Example1 n={1} />, container);
expect(container.textContent).toBe('1');
expect(container2.textContent).toBe('');
expect(container3.textContent).toBe('');
Scheduler.unstable_flushAll();
expect(container.textContent).toBe('1');
expect(container2.textContent).toBe('2');
expect(container3.textContent).toBe('3');

ReactDOM.render(<Example1 n={2} />, container);
expect(container.textContent).toBe('2');
expect(container2.textContent).toBe('2'); // Not flushed yet
expect(container3.textContent).toBe('3'); // Not flushed yet
Scheduler.unstable_flushAll();
expect(container.textContent).toBe('2');
expect(container2.textContent).toBe('4');
expect(container3.textContent).toBe('6');
}).toWarnDev([
'An update to Example1 ran an effect',
'An update to Example2 ran an effect',
'An update to Example1 ran an effect',
'An update to Example2 ran an effect',
]);
ReactDOM.render(<Example1 n={1} />, container);
expect(container.textContent).toBe('1');
expect(container2.textContent).toBe('');
expect(container3.textContent).toBe('');
Scheduler.unstable_flushAll();
expect(container.textContent).toBe('1');
expect(container2.textContent).toBe('2');
expect(container3.textContent).toBe('3');

ReactDOM.render(<Example1 n={2} />, container);
expect(container.textContent).toBe('2');
expect(container2.textContent).toBe('2'); // Not flushed yet
expect(container3.textContent).toBe('3'); // Not flushed yet
Scheduler.unstable_flushAll();
expect(container.textContent).toBe('2');
expect(container2.textContent).toBe('4');
expect(container3.textContent).toBe('6');
});

it('should not bail out when an update is scheduled from within an event handler', () => {
@@ -48,6 +48,20 @@ describe('ReactTestUtils.act()', () => {
ReactDOM.unmountComponentAtNode(dom);
}
runActTests('legacy sync mode', renderSync, unmountSync);

// and then in batched mode
let batchedRoot;
function renderBatched(el, dom) {
batchedRoot = ReactDOM.unstable_createSyncRoot(dom);
batchedRoot.render(el);
}
function unmountBatched(dom) {
if (batchedRoot !== null) {
batchedRoot.unmount();
batchedRoot = null;
}
}
runActTests('batched mode', renderBatched, unmountBatched);
});

function runActTests(label, render, unmount) {
@@ -68,6 +82,26 @@ function runActTests(label, render, unmount) {
document.body.removeChild(container);
});
describe('sync', () => {
it('warns if an effect is queued outside an act scope, except in legacy sync+non-strict mode', () => {
function App() {
React.useEffect(() => {}, []);
return null;
}
expect(() => {
render(<App />, container);
// flush all queued work
Scheduler.unstable_flushAll();
}).toWarnDev(
label !== 'legacy sync mode'
This conversation was marked as resolved by threepointone

This comment has been minimized.

Copy link
@acdlite

acdlite Jul 2, 2019

Member

:headdesk: This is why I don't like abstractions in tests

How about we write a separate test for each mode? :D

? [
// warns twice because we're in strict+dev mode
'An update to App ran an effect, but was not wrapped in act(...)',
'An update to App ran an effect, but was not wrapped in act(...)',
]
: [],
);
});

it('can use act to flush effects', () => {
function App() {
React.useEffect(() => {
@@ -2453,6 +2453,7 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void {
if (__DEV__) {
if (
warnsIfNotActing === true &&
fiber.mode &&
This conversation was marked as resolved by threepointone

This comment has been minimized.

Copy link
@acdlite

acdlite Jul 2, 2019

Member

Do an explicit comparison for the modes we care about, please, so it doesn't accidentally break if we add another mode. Also makes it greppable.

This comment has been minimized.

Copy link
@threepointone

threepointone Jul 2, 2019

Author Contributor

the condition is for anything but sync mode (including future modes?). but yeah, maybe better to be obvious.

This comment has been minimized.

Copy link
@acdlite

acdlite Jul 2, 2019

Member

Probably, but can't say for sure which modes we might add the future. Maybe the next one won't even be scheduling related.

IsSomeRendererActing.current === false &&
IsThisRendererActing.current === false
) {
@@ -1806,7 +1806,6 @@ describe('ReactHooks', () => {
globalListener();
globalListener();
}).toWarnDev([
'An update to C ran an effect',
'An update to C inside a test was not wrapped in act',
'An update to C inside a test was not wrapped in act',
// Note: should *not* warn about updates on unmounted component.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.