Skip to content

Commit

Permalink
warn if passive effects get queued outside of an act() call. (#15763)
Browse files Browse the repository at this point in the history
* warn if passive effects get queued outside of an act() call

While the code itself isn't much (it adds the warning to mountEffect() and updateEffect() in ReactFiberHooks), it does change a lot of our tests. We follow a bad-ish pattern here, which is doing asserts inside act() scopes, but it makes sense for *us* because we're testing intermediate states, and we're manually flush/yield what we need in these tests.

This commit has one last failing test. Working on it.

* pass lint

* pass failing test, fixes another

- a test was failing in ReactDOMServerIntegrationHooks while testing an effect; the behaviour of yields was different from browser and server when wrapped with act(). further, because of how we initialized modules, act() around renders wasn't working corrrectly. solved by passing in ReactTestUtils in initModules, and checking on the finally yielded values in the specific test.
- in ReactUpdates, while testing an infinite recursion detection, the test needed to be wrapped in an act(), which would have caused the recusrsion error to throw. solived by rethrowing the error from inside the act().

* pass ReactDOMServerSuspense

* stray todo

* a better message, consistent with the state update one.
  • Loading branch information
Sunil Pai committed Jun 24, 2019
1 parent 39b97e8 commit e1c5e87
Show file tree
Hide file tree
Showing 35 changed files with 1,005 additions and 732 deletions.
Expand Up @@ -146,7 +146,10 @@ describe('ReactHooksInspectionIntegration', () => {
</div>
);
}
let renderer = ReactTestRenderer.create(<Foo prop="prop" />);
let renderer;
act(() => {
renderer = ReactTestRenderer.create(<Foo prop="prop" />);
});

let childFiber = renderer.root.findByType(Foo)._currentFiber();

Expand Down
43 changes: 26 additions & 17 deletions packages/react-dom/src/__tests__/ReactDOMHooks-test.js
Expand Up @@ -53,23 +53,32 @@ describe('ReactDOMHooks', () => {
return 3 * n;
}

ReactDOM.render(<Example1 n={1} />, container);
expect(container.textContent).toBe('1');
expect(container2.textContent).toBe('');
expect(container3.textContent).toBe('');
Scheduler.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.flushAll();
expect(container.textContent).toBe('2');
expect(container2.textContent).toBe('4');
expect(container3.textContent).toBe('6');
// 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.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.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',
]);
});

it('should not bail out when an update is scheduled from within an event handler', () => {
Expand Down
Expand Up @@ -13,6 +13,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegratio

let React;
let ReactDOM;
let ReactTestUtils;
let ReactDOMServer;

function initModules() {
Expand All @@ -21,11 +22,13 @@ function initModules() {
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');

// Make them available to the helpers.
return {
ReactDOM,
ReactDOMServer,
ReactTestUtils,
};
}

Expand Down
Expand Up @@ -16,18 +16,21 @@ const TEXT_NODE_TYPE = 3;
let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;

function initModules() {
// Reset warning cache.
jest.resetModuleRegistry();
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');

// Make them available to the helpers.
return {
ReactDOM,
ReactDOMServer,
ReactTestUtils,
};
}

Expand Down
Expand Up @@ -16,18 +16,21 @@ const {disableInputAttributeSyncing} = require('shared/ReactFeatureFlags');
let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;

function initModules() {
// Reset warning cache.
jest.resetModuleRegistry();
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');

// Make them available to the helpers.
return {
ReactDOM,
ReactDOMServer,
ReactTestUtils,
};
}

Expand Down
Expand Up @@ -14,18 +14,21 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegratio
let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;

function initModules() {
// Reset warning cache.
jest.resetModuleRegistry();
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');

// Make them available to the helpers.
return {
ReactDOM,
ReactDOMServer,
ReactTestUtils,
};
}

Expand Down
Expand Up @@ -16,17 +16,20 @@ const TEXT_NODE_TYPE = 3;
let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;

function initModules() {
jest.resetModuleRegistry();
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');

// Make them available to the helpers.
return {
ReactDOM,
ReactDOMServer,
ReactTestUtils,
};
}

Expand Down
Expand Up @@ -14,18 +14,21 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegratio
let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;

function initModules() {
// Reset warning cache.
jest.resetModuleRegistry();
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');

// Make them available to the helpers.
return {
ReactDOM,
ReactDOMServer,
ReactTestUtils,
};
}

Expand Down
Expand Up @@ -17,6 +17,7 @@ let React;
let ReactFeatureFlags;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;
let useState;
let useReducer;
let useEffect;
Expand All @@ -41,6 +42,7 @@ function initModules() {
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');
useState = React.useState;
useReducer = React.useReducer;
useEffect = React.useEffect;
Expand All @@ -67,6 +69,7 @@ function initModules() {
return {
ReactDOM,
ReactDOMServer,
ReactTestUtils,
};
}

Expand Down Expand Up @@ -539,18 +542,30 @@ describe('ReactDOMServerHooks', () => {
});

describe('useEffect', () => {
const yields = [];
itRenders('should ignore effects on the server', async render => {
function Counter(props) {
useEffect(() => {
yieldValue('should not be invoked');
yieldValue('invoked on client');
});
return <Text text={'Count: ' + props.count} />;
}

const domNode = await render(<Counter count={0} />);
expect(clearYields()).toEqual(['Count: 0']);
yields.push(clearYields());
expect(domNode.tagName).toEqual('SPAN');
expect(domNode.textContent).toEqual('Count: 0');
});

it('verifies yields in order', () => {
expect(yields).toEqual([
['Count: 0'], // server render
['Count: 0'], // server stream
['Count: 0', 'invoked on client'], // clean render
['Count: 0', 'invoked on client'], // hydrated render
// nothing yielded for bad markup
]);
});
});

describe('useCallback', () => {
Expand Down
Expand Up @@ -16,18 +16,21 @@ const {disableInputAttributeSyncing} = require('shared/ReactFeatureFlags');
let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;

function initModules() {
// Reset warning cache.
jest.resetModuleRegistry();
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');

// Make them available to the helpers.
return {
ReactDOM,
ReactDOMServer,
ReactTestUtils,
};
}

Expand Down
Expand Up @@ -15,6 +15,7 @@ let PropTypes;
let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;

function initModules() {
// Reset warning cache.
Expand All @@ -23,11 +24,13 @@ function initModules() {
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');

// Make them available to the helpers.
return {
ReactDOM,
ReactDOMServer,
ReactTestUtils,
};
}

Expand Down
Expand Up @@ -14,18 +14,21 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegratio
let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;

function initModules() {
// Reset warning cache.
jest.resetModuleRegistry();
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');

// Make them available to the helpers.
return {
ReactDOM,
ReactDOMServer,
ReactTestUtils,
};
}

Expand Down
Expand Up @@ -14,18 +14,21 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegratio
let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;

function initModules() {
// Reset warning cache.
jest.resetModuleRegistry();
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');

// Make them available to the helpers.
return {
ReactDOM,
ReactDOMServer,
ReactTestUtils,
};
}

Expand Down
Expand Up @@ -14,6 +14,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegratio
let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;

function initModules() {
// Reset warning cache.
Expand All @@ -22,11 +23,13 @@ function initModules() {
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');

// Make them available to the helpers.
return {
ReactDOM,
ReactDOMServer,
ReactTestUtils,
};
}

Expand Down
Expand Up @@ -14,18 +14,21 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegratio
let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;

function initModules() {
// Reset warning cache.
jest.resetModuleRegistry();
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');

// Make them available to the helpers.
return {
ReactDOM,
ReactDOMServer,
ReactTestUtils,
};
}

Expand Down

0 comments on commit e1c5e87

Please sign in to comment.