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

Jump to bottom

Disable console.logs in the second render pass of DEV mode double render #18547

Merged
merged 3 commits into from Apr 8, 2020
Merged

Disable console.logs in the second render pass of DEV mode double render #18547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
057e4a0
Disable console log during the second rerender
sebmarkbage Apr 8, 2020
2d742e5
Use the disabled log to avoid double yielding values in scheduler mock
sebmarkbage Apr 8, 2020
b7e2345
Reenable debugRenderPhaseSideEffectsForStrictMode in tests that can
sebmarkbage Apr 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 1 addition & 3 deletions packages/create-subscription/src/__tests__/createSubscription-test.internal.js
View file
Open in desktop
Expand Up @@ -11,7 +11,6 @@

let createSubscription;
let BehaviorSubject;
let ReactFeatureFlags;
let React;
let ReactNoop;
let Scheduler;
Expand All @@ -21,8 +20,7 @@ describe('createSubscription', () => {
beforeEach(() => {
jest.resetModules();
createSubscription = require('create-subscription').createSubscription;
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;

React = require('react');
ReactNoop = require('react-noop-renderer');
Scheduler = require('scheduler');
Expand Down
6 changes: 2 additions & 4 deletions packages/react-art/src/__tests__/ReactART-test.js
View file
Open in desktop
Expand Up @@ -391,7 +391,7 @@ describe('ReactART', () => {
</CurrentRendererContext.Provider>,
);

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

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

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

expect(ops).toEqual(['Test']);
});
Expand Down
2 changes: 1 addition & 1 deletion packages/react-cache/src/__tests__/ReactCache-test.internal.js
View file
Open in desktop
Expand Up @@ -24,7 +24,7 @@ describe('ReactCache', () => {
jest.resetModules();

ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;

ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
React = require('react');
Suspense = React.Suspense;
Expand Down
8 changes: 2 additions & 6 deletions packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.internal.js
View file
Open in desktop
Expand Up @@ -10,7 +10,6 @@
'use strict';

let React;
let ReactFeatureFlags = require('shared/ReactFeatureFlags');

let ReactDOM;
let Scheduler;
Expand Down Expand Up @@ -149,8 +148,7 @@ describe('ReactDOMFiberAsync', () => {
describe('concurrent mode', () => {
beforeEach(() => {
jest.resetModules();
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can remove internal suffix 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.

Yea I'll do a follow up to rename all these if I land this. I think the remaining ones are just using the let ops = [] pattern and we can port those.


ReactDOM = require('react-dom');
Scheduler = require('scheduler');
});
Expand Down Expand Up @@ -639,9 +637,7 @@ describe('ReactDOMFiberAsync', () => {
expect(container.textContent).toEqual('');

// Everything should render immediately in the next event
expect(Scheduler).toFlushExpired(
__DEV__ ? ['A', 'A', 'B', 'B', 'C', 'C'] : ['A', 'B', 'C'],
);
expect(Scheduler).toFlushExpired(['A', 'B', 'C']);
expect(container.textContent).toEqual('ABC');
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js
View file
Open in desktop
Expand Up @@ -40,7 +40,7 @@ function initModules() {
jest.resetModuleRegistry();

ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;

ReactFeatureFlags.flushSuspenseFallbacksInTests = false;
React = require('react');
ReactDOM = require('react-dom');
Expand Down
1 change: 0 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
View file
Open in desktop
Expand Up @@ -77,7 +77,6 @@ describe('ReactDOMServerPartialHydration', () => {
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.enableSuspenseCallback = true;
ReactFeatureFlags.enableDeprecatedFlareAPI = true;
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;

React = require('react');
ReactDOM = require('react-dom');
Expand Down
1 change: 0 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js
View file
Open in desktop
Expand Up @@ -97,7 +97,6 @@ describe('ReactDOMServerSelectiveHydration', () => {

const ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.enableDeprecatedFlareAPI = true;
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;

React = require('react');
ReactDOM = require('react-dom');
Expand Down
3 changes: 0 additions & 3 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
View file
Open in desktop
Expand Up @@ -121,7 +121,6 @@ describe('ReactTestUtils.act()', () => {
);
}).toErrorDev([
'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(...)',
]);
});

Expand All @@ -132,7 +131,6 @@ describe('ReactTestUtils.act()', () => {
Scheduler.unstable_flushAll();
}).toErrorDev([
'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(...)',
]);
});

Expand All @@ -143,7 +141,6 @@ describe('ReactTestUtils.act()', () => {
Scheduler.unstable_flushAll();
}).toErrorDev([
'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(...)',
]);
});
});
Expand Down
28 changes: 3 additions & 25 deletions packages/react-dom/src/__tests__/ReactUpdates-test.js
View file
Open in desktop
Expand Up @@ -1324,30 +1324,12 @@ describe('ReactUpdates', () => {
let hiddenDiv;
act(() => {
root.render(<Foo />);
if (__DEV__) {
expect(Scheduler).toFlushAndYieldThrough([
'Foo',
'Foo',
'Baz',
'Baz',
'Foo#effect',
]);
} else {
expect(Scheduler).toFlushAndYieldThrough([
'Foo',
'Baz',
'Foo#effect',
]);
}
expect(Scheduler).toFlushAndYieldThrough(['Foo', 'Baz', 'Foo#effect']);
hiddenDiv = container.firstChild.firstChild;
expect(hiddenDiv.hidden).toBe(true);
expect(hiddenDiv.innerHTML).toBe('');
// Run offscreen update
if (__DEV__) {
expect(Scheduler).toFlushAndYield(['Bar', 'Bar']);
} else {
expect(Scheduler).toFlushAndYield(['Bar']);
}
expect(Scheduler).toFlushAndYield(['Bar']);
expect(hiddenDiv.hidden).toBe(true);
expect(hiddenDiv.innerHTML).toBe('<p>bar 0</p>');
});
Expand All @@ -1359,11 +1341,7 @@ describe('ReactUpdates', () => {
expect(hiddenDiv.innerHTML).toBe('<p>bar 0</p>');

// Run offscreen update
if (__DEV__) {
expect(Scheduler).toFlushAndYield(['Bar', 'Bar']);
} else {
expect(Scheduler).toFlushAndYield(['Bar']);
}
expect(Scheduler).toFlushAndYield(['Bar']);
expect(hiddenDiv.innerHTML).toBe('<p>bar 1</p>');
},
);
Expand Down
14 changes: 2 additions & 12 deletions packages/react-dom/src/events/__tests__/DOMModernPluginEventSystem-test.internal.js
View file
Open in desktop
Expand Up @@ -1701,12 +1701,7 @@ describe('DOMModernPluginEventSystem', () => {
const root = ReactDOM.createRoot(container);
root.render(<Test counter={0} />);

// Dev double-render
if (__DEV__) {
expect(Scheduler).toFlushAndYield(['Test', 'Test']);
} else {
expect(Scheduler).toFlushAndYield(['Test']);
}
expect(Scheduler).toFlushAndYield(['Test']);

// Click the button
dispatchClickEvent(ref.current);
Expand All @@ -1718,12 +1713,7 @@ describe('DOMModernPluginEventSystem', () => {
// Increase counter
root.render(<Test counter={1} />);
// Yield before committing
// Dev double-render
if (__DEV__) {
expect(Scheduler).toFlushAndYieldThrough(['Test', 'Test']);
} else {
expect(Scheduler).toFlushAndYieldThrough(['Test']);
}
expect(Scheduler).toFlushAndYieldThrough(['Test']);

// Click the button again
dispatchClickEvent(ref.current);
Expand Down
6 changes: 2 additions & 4 deletions packages/react-dom/src/events/__tests__/DeprecatedDOMEventResponderSystem-test.internal.js
View file
Open in desktop
Expand Up @@ -811,7 +811,7 @@ describe('DOMEventResponderSystem', () => {

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

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

// Click the button again
dispatchClickEvent(ref.current);
Expand Down
93 changes: 60 additions & 33 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
View file
Open in desktop
Expand Up @@ -181,6 +181,8 @@ import {
getWorkInProgressRoot,
} from './ReactFiberWorkLoop';

import {disableLogs, reenableLogs} from 'shared/ConsolePatchingDev';

const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;

let didReceiveUpdate: boolean = false;
Expand Down Expand Up @@ -320,14 +322,19 @@ function updateForwardRef(
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictMode
) {
nextChildren = renderWithHooks(
current,
workInProgress,
render,
nextProps,
ref,
renderExpirationTime,
);
disableLogs();
try {
nextChildren = renderWithHooks(
current,
workInProgress,
render,
nextProps,
ref,
renderExpirationTime,
);
} finally {
reenableLogs();
}
}
setIsRendering(false);
} else {
Expand Down Expand Up @@ -658,14 +665,19 @@ function updateFunctionComponent(
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictMode
) {
nextChildren = renderWithHooks(
current,
workInProgress,
Component,
nextProps,
context,
renderExpirationTime,
);
disableLogs();
try {
nextChildren = renderWithHooks(
current,
workInProgress,
Component,
nextProps,
context,
renderExpirationTime,
);
} finally {
reenableLogs();
}
}
setIsRendering(false);
} else {
Expand Down Expand Up @@ -731,14 +743,19 @@ function updateBlock<Props, Data>(
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictMode
) {
nextChildren = renderWithHooks(
current,
workInProgress,
render,
nextProps,
data,
renderExpirationTime,
);
disableLogs();
try {
nextChildren = renderWithHooks(
current,
workInProgress,
render,
nextProps,
data,
renderExpirationTime,
);
} finally {
reenableLogs();
}
}
setIsRendering(false);
} else {
Expand Down Expand Up @@ -923,7 +940,12 @@ function finishClassComponent(
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictMode
) {
instance.render();
disableLogs();
try {
instance.render();
} finally {
reenableLogs();
}
}
setIsRendering(false);
} else {
Expand Down Expand Up @@ -1485,14 +1507,19 @@ function mountIndeterminateComponent(
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictMode
) {
value = renderWithHooks(
null,
workInProgress,
Component,
props,
context,
renderExpirationTime,
);
disableLogs();
try {
value = renderWithHooks(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I could foresee some confusion if the logs don't correspond to the value that is returned. It usually doesn't matter because it's supposed to be pure, but in the case where it's not, you might use console to debug it. So maybe they should be disabled for the first pass instead of the second.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I was actually surprised that we used the second value. I was thinking we should ignore the second value like we do for class constructors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my other PR's replaying I have to ignore it because I force an error inside the function. Similarly the react-debug-tools for Hooks debugging it's just replayed stand-alone.

Doesn't necessarily mean we have to do the same here. I guess that's how I look at these.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's intentional because the children are bound to the work-in-progress fiber via the queue object via the dispatch method, and renderWithHooks mutates the fiber.

It was a very confusing bug when we found it :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

This one: #14698

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could go either way on if the second render should process the update queue and even leave any side-effects. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe the second render should have a different dispatcher that warns if you call setState again since you shouldn't call setState again for the same input.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I could go either way on if the second render should process the update queue and even leave any side-effects :)

It can work but it needs to include internal mutations/side-effects too. We'd have to make it so that renderWithHooks does not mutate any of the fiber fields. Or stash the result of the first pass (memoizedState, expirationTime, effectTag) before doing the second pass and then restore them.

Copy link
Collaborator

@acdlite acdlite Apr 8, 2020 •
edited

Choose a reason for hiding this comment

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

Maybe the second render should have a different dispatcher that warns if you call setState again since you shouldn't call setState again for the same input.

Oh I didn't see that. Yeah I think that would work. Just call the component again with a special dispatcher that doesn't mutate anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the second render should have a different dispatcher that warns if you call setState again since you shouldn't call setState again for the same input.

I might be misunderstanding this comment, but if the second call has state updates already applied, then we might miss out on triggering the observable side effects in the first place (e.g. like something that happens only when props/state meet a certain criteria).

null,
workInProgress,
Component,
props,
context,
renderExpirationTime,
);
} finally {
reenableLogs();
}
}
}
reconcileChildren(null, workInProgress, value, renderExpirationTime);
Expand Down