Skip to content

Commit

Permalink
feat(StrictMode): Double-invoke render for every component (#18430)
Browse files Browse the repository at this point in the history 
* feat(StrictMode): Double-invoke render for every component

* fix: Mark ReactTestRendererAsync as internal
  • Loading branch information
@eps1lon
eps1lon committed Mar 29, 2020
1 parent 689d275 commit ba31ad4
Show file tree
Hide file tree
Showing 12 changed files with 76 additions and 73 deletions.
6 changes: 4 additions & 2 deletions packages/react-art/src/__tests__/ReactART-test.js
View file
Expand Up @@ -391,7 +391,7 @@ describe('ReactART', () => {
</CurrentRendererContext.Provider>,
);

expect(Scheduler).toFlushAndYieldThrough(['A']);
expect(Scheduler).toFlushAndYieldThrough(__DEV__ ? ['A', 'A'] : ['A']);

ReactDOM.render(
<Surface>
Expand All @@ -406,7 +406,9 @@ describe('ReactART', () => {
expect(ops).toEqual([null, 'ART']);

ops = [];
expect(Scheduler).toFlushAndYield(['B', 'C']);
expect(Scheduler).toFlushAndYield(
__DEV__ ? ['B', 'B', 'C', 'C'] : ['B', 'C'],
);

expect(ops).toEqual(['Test']);
});
Expand Down
4 changes: 3 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.internal.js
View file
Expand Up @@ -639,7 +639,9 @@ describe('ReactDOMFiberAsync', () => {
expect(container.textContent).toEqual('');

// Everything should render immediately in the next event
expect(Scheduler).toFlushExpired(['A', 'B', 'C']);
expect(Scheduler).toFlushExpired(
__DEV__ ? ['A', 'A', 'B', 'B', 'C', 'C'] : ['A', 'B', 'C'],
);
expect(container.textContent).toEqual('ABC');
});
});
Expand Down
4 changes: 3 additions & 1 deletion packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
View file
Expand Up @@ -607,7 +607,9 @@ function runActTests(label, render, unmount, rerender) {
},
);

expect(Component).toHaveBeenCalledTimes(4);
expect(Component).toHaveBeenCalledTimes(
label === 'legacy mode' ? 4 : 8,
);
unmount(secondContainer);
});
}
Expand Down
1 change: 1 addition & 0 deletions packages/react-dom/src/__tests__/ReactUpdates-test.js
View file
Expand Up @@ -1333,6 +1333,7 @@ describe('ReactUpdates', () => {
'Foo',
'Foo',
'Baz',
'Baz',
'Foo#effect',
]);
} else {
Expand Down
6 changes: 4 additions & 2 deletions packages/react-dom/src/events/__tests__/DeprecatedDOMEventResponderSystem-test.internal.js
View file
Expand Up @@ -813,7 +813,7 @@ describe('DOMEventResponderSystem', () => {

let root = ReactDOM.createRoot(container);
root.render(<Test counter={0} />);
expect(Scheduler).toFlushAndYield(['Test']);
expect(Scheduler).toFlushAndYield(__DEV__ ? ['Test', 'Test'] : ['Test']);

// Click the button
dispatchClickEvent(ref.current);
Expand All @@ -825,7 +825,9 @@ describe('DOMEventResponderSystem', () => {
// Increase counter
root.render(<Test counter={1} />);
// Yield before committing
expect(Scheduler).toFlushAndYieldThrough(['Test']);
expect(Scheduler).toFlushAndYieldThrough(
__DEV__ ? ['Test', 'Test'] : ['Test'],
);

// Click the button again
dispatchClickEvent(ref.current);
Expand Down
76 changes: 32 additions & 44 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
View file
Expand Up @@ -321,17 +321,14 @@ function updateForwardRef(
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictMode
) {
// Only double-render components with Hooks
if (workInProgress.memoizedState !== null) {
nextChildren = renderWithHooks(
current,
workInProgress,
render,
nextProps,
ref,
renderExpirationTime,
);
}
nextChildren = renderWithHooks(
current,
workInProgress,
render,
nextProps,
ref,
renderExpirationTime,
);
}
setIsRendering(false);
} else {
Expand Down Expand Up @@ -662,17 +659,14 @@ function updateFunctionComponent(
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictMode
) {
// Only double-render components with Hooks
if (workInProgress.memoizedState !== null) {
nextChildren = renderWithHooks(
current,
workInProgress,
Component,
nextProps,
context,
renderExpirationTime,
);
}
nextChildren = renderWithHooks(
current,
workInProgress,
Component,
nextProps,
context,
renderExpirationTime,
);
}
setIsRendering(false);
} else {
Expand Down Expand Up @@ -738,17 +732,14 @@ function updateBlock<Props, Data>(
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictMode
) {
// Only double-render components with Hooks
if (workInProgress.memoizedState !== null) {
nextChildren = renderWithHooks(
current,
workInProgress,
render,
nextProps,
data,
renderExpirationTime,
);
}
nextChildren = renderWithHooks(
current,
workInProgress,
render,
nextProps,
data,
renderExpirationTime,
);
}
setIsRendering(false);
} else {
Expand Down Expand Up @@ -1471,17 +1462,14 @@ function mountIndeterminateComponent(
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictMode
) {
// Only double-render components with Hooks
if (workInProgress.memoizedState !== null) {
value = renderWithHooks(
null,
workInProgress,
Component,
props,
context,
renderExpirationTime,
);
}
value = renderWithHooks(
null,
workInProgress,
Component,
props,
context,
renderExpirationTime,
);
}
}
reconcileChildren(null, workInProgress, value, renderExpirationTime);
Expand Down
1 change: 1 addition & 0 deletions packages/react-reconciler/src/__tests__/ErrorBoundaryReconciliation-test.internal.js
View file
Expand Up @@ -12,6 +12,7 @@ describe('ErrorBoundaryReconciliation', () => {
jest.resetModules();

ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
ReactTestRenderer = require('react-test-renderer');
React = require('react');
Expand Down
12 changes: 6 additions & 6 deletions packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
View file
Expand Up @@ -1300,14 +1300,14 @@ describe('ReactHooks', () => {
<NoHooks />
</StrictMode>,
);
expect(renderCount).toBe(1);
expect(renderCount).toBe(__DEV__ ? 2 : 1);
renderCount = 0;
renderer.update(
<StrictMode>
<NoHooks />
</StrictMode>,
);
expect(renderCount).toBe(1);
expect(renderCount).toBe(__DEV__ ? 2 : 1);

renderCount = 0;
renderer.update(<FwdRef />);
Expand All @@ -1321,14 +1321,14 @@ describe('ReactHooks', () => {
<FwdRef />
</StrictMode>,
);
expect(renderCount).toBe(1);
expect(renderCount).toBe(__DEV__ ? 2 : 1);
renderCount = 0;
renderer.update(
<StrictMode>
<FwdRef />
</StrictMode>,
);
expect(renderCount).toBe(1);
expect(renderCount).toBe(__DEV__ ? 2 : 1);

renderCount = 0;
renderer.update(<Memo arg={1} />);
Expand All @@ -1342,14 +1342,14 @@ describe('ReactHooks', () => {
<Memo arg={1} />
</StrictMode>,
);
expect(renderCount).toBe(1);
expect(renderCount).toBe(__DEV__ ? 2 : 1);
renderCount = 0;
renderer.update(
<StrictMode>
<Memo arg={2} />
</StrictMode>,
);
expect(renderCount).toBe(1);
expect(renderCount).toBe(__DEV__ ? 2 : 1);

renderCount = 0;
expect(() => renderer.update(<Factory />)).toErrorDev(
Expand Down
4 changes: 3 additions & 1 deletion packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js
View file
Expand Up @@ -1103,7 +1103,9 @@ describe('ReactNewContext', () => {

// Render the provider again using a different renderer
ReactNoop.render(<App value={1} />);
expect(Scheduler).toFlushAndYield(['Foo', 'Foo']);
expect(Scheduler).toFlushAndYield(
__DEV__ ? ['Foo', 'Foo', 'Foo', 'Foo'] : ['Foo', 'Foo'],
);

if (__DEV__) {
expect(console.error.calls.argsFor(0)[0]).toContain(
Expand Down
3 changes: 3 additions & 0 deletions .../__tests__/ReactTestRendererAsync-test.js → ..._/ReactTestRendererAsync-test.internal.js
View file
Expand Up @@ -11,12 +11,15 @@
'use strict';

let React;
let ReactFeatureFlags;
let ReactTestRenderer;
let Scheduler;

describe('ReactTestRendererAsync', () => {
beforeEach(() => {
jest.resetModules();
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
React = require('react');
ReactTestRenderer = require('react-test-renderer');
Scheduler = require('scheduler');
Expand Down
4 changes: 2 additions & 2 deletions packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js
View file
Expand Up @@ -188,7 +188,7 @@ describe('ReactProfiler DevTools integration', () => {

// Commit something
root.update(<Text text="A" />);
expect(Scheduler).toFlushAndYield(['A']);
expect(Scheduler).toFlushAndYield(__DEV__ ? ['A', 'A'] : ['A']);
expect(root).toMatchRenderedOutput('A');

// Advance time by many seconds, larger than the default expiration time
Expand All @@ -200,7 +200,7 @@ describe('ReactProfiler DevTools integration', () => {
// Update B should not instantly expire.
expect(Scheduler).toFlushExpired([]);

expect(Scheduler).toFlushAndYield(['B']);
expect(Scheduler).toFlushAndYield(__DEV__ ? ['B', 'B'] : ['B']);
expect(root).toMatchRenderedOutput('B');
});
});
28 changes: 14 additions & 14 deletions packages/react/src/__tests__/forwardRef-test.js
View file
Expand Up @@ -260,11 +260,11 @@ describe('forwardRef', () => {

ReactNoop.render(<RefForwardingComponent ref={ref} optional="foo" />);
expect(Scheduler).toFlushWithoutYielding();
expect(renderCount).toBe(1);
expect(renderCount).toBe(__DEV__ ? 2 : 1);

ReactNoop.render(<RefForwardingComponent ref={ref} optional="foo" />);
expect(Scheduler).toFlushWithoutYielding();
expect(renderCount).toBe(2);
expect(renderCount).toBe(__DEV__ ? 4 : 2);
});

it('should bailout if forwardRef is wrapped in memo', () => {
Expand All @@ -283,28 +283,28 @@ describe('forwardRef', () => {

ReactNoop.render(<RefForwardingComponent ref={ref} optional="foo" />);
expect(Scheduler).toFlushWithoutYielding();
expect(renderCount).toBe(1);
expect(renderCount).toBe(__DEV__ ? 2 : 1);

expect(ref.current.type).toBe('div');

ReactNoop.render(<RefForwardingComponent ref={ref} optional="foo" />);
expect(Scheduler).toFlushWithoutYielding();
expect(renderCount).toBe(1);
expect(renderCount).toBe(__DEV__ ? 2 : 1);

const differentRef = React.createRef();

ReactNoop.render(
<RefForwardingComponent ref={differentRef} optional="foo" />,
);
expect(Scheduler).toFlushWithoutYielding();
expect(renderCount).toBe(2);
expect(renderCount).toBe(__DEV__ ? 4 : 2);

expect(ref.current).toBe(null);
expect(differentRef.current.type).toBe('div');

ReactNoop.render(<RefForwardingComponent ref={ref} optional="bar" />);
expect(Scheduler).toFlushWithoutYielding();
expect(renderCount).toBe(3);
expect(renderCount).toBe(__DEV__ ? 6 : 3);
});

it('should custom memo comparisons to compose', () => {
Expand All @@ -324,19 +324,19 @@ describe('forwardRef', () => {

ReactNoop.render(<RefForwardingComponent ref={ref} a="0" b="0" c="1" />);
expect(Scheduler).toFlushWithoutYielding();
expect(renderCount).toBe(1);
expect(renderCount).toBe(__DEV__ ? 2 : 1);

expect(ref.current.type).toBe('div');

// Changing either a or b rerenders
ReactNoop.render(<RefForwardingComponent ref={ref} a="0" b="1" c="1" />);
expect(Scheduler).toFlushWithoutYielding();
expect(renderCount).toBe(2);
expect(renderCount).toBe(__DEV__ ? 4 : 2);

// Changing c doesn't rerender
ReactNoop.render(<RefForwardingComponent ref={ref} a="0" b="1" c="2" />);
expect(Scheduler).toFlushWithoutYielding();
expect(renderCount).toBe(2);
expect(renderCount).toBe(__DEV__ ? 4 : 2);

const ComposedMemo = React.memo(
RefForwardingComponent,
Expand All @@ -345,29 +345,29 @@ describe('forwardRef', () => {

ReactNoop.render(<ComposedMemo ref={ref} a="0" b="0" c="0" />);
expect(Scheduler).toFlushWithoutYielding();
expect(renderCount).toBe(3);
expect(renderCount).toBe(__DEV__ ? 6 : 3);

// Changing just b no longer updates
ReactNoop.render(<ComposedMemo ref={ref} a="0" b="1" c="0" />);
expect(Scheduler).toFlushWithoutYielding();
expect(renderCount).toBe(3);
expect(renderCount).toBe(__DEV__ ? 6 : 3);

// Changing just a and c updates
ReactNoop.render(<ComposedMemo ref={ref} a="2" b="2" c="2" />);
expect(Scheduler).toFlushWithoutYielding();
expect(renderCount).toBe(4);
expect(renderCount).toBe(__DEV__ ? 8 : 4);

// Changing just c does not update
ReactNoop.render(<ComposedMemo ref={ref} a="2" b="2" c="3" />);
expect(Scheduler).toFlushWithoutYielding();
expect(renderCount).toBe(4);
expect(renderCount).toBe(__DEV__ ? 8 : 4);

// Changing ref still rerenders
const differentRef = React.createRef();

ReactNoop.render(<ComposedMemo ref={differentRef} a="2" b="2" c="3" />);
expect(Scheduler).toFlushWithoutYielding();
expect(renderCount).toBe(5);
expect(renderCount).toBe(__DEV__ ? 10 : 5);

expect(ref.current).toBe(null);
expect(differentRef.current.type).toBe('div');
Expand Down

0 comments on commit ba31ad4

Please sign in to comment.