Skip to content

Commit 51b0bec

Browse files
gaearongaearon
authored
Always keep disabled logs in the second pass (#21739)
* Add tests for disabled logs * Always keep disabled logs in the second pass * Jest nit * Always use the second result
1 parent 386e8f2 commit 51b0bec

File tree

3 files changed

+220
-36
lines changed

3 files changed

+220
-36
lines changed

‎packages/react-reconciler/src/ReactFiberClassComponent.new.js

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ function applyDerivedStateFromProps(
169169
nextProps: any,
170170
) {
171171
const prevState = workInProgress.memoizedState;
172-
172+
let partialState = getDerivedStateFromProps(nextProps, prevState);
173173
if (__DEV__) {
174174
if (
175175
debugRenderPhaseSideEffectsForStrictMode &&
@@ -178,16 +178,11 @@ function applyDerivedStateFromProps(
178178
disableLogs();
179179
try {
180180
// Invoke the function an extra time to help detect side-effects.
181-
getDerivedStateFromProps(nextProps, prevState);
181+
partialState = getDerivedStateFromProps(nextProps, prevState);
182182
} finally {
183183
reenableLogs();
184184
}
185185
}
186-
}
187-
188-
const partialState = getDerivedStateFromProps(nextProps, prevState);
189-
190-
if (__DEV__) {
191186
warnOnUndefinedDerivedState(ctor, partialState);
192187
}
193188
// Merge the partial state and the previous state.
@@ -323,6 +318,11 @@ function checkShouldComponentUpdate(
323318
) {
324319
const instance = workInProgress.stateNode;
325320
if (typeof instance.shouldComponentUpdate === 'function') {
321+
let shouldUpdate = instance.shouldComponentUpdate(
322+
newProps,
323+
newState,
324+
nextContext,
325+
);
326326
if (__DEV__) {
327327
if (
328328
debugRenderPhaseSideEffectsForStrictMode &&
@@ -331,19 +331,15 @@ function checkShouldComponentUpdate(
331331
disableLogs();
332332
try {
333333
// Invoke the function an extra time to help detect side-effects.
334-
instance.shouldComponentUpdate(newProps, newState, nextContext);
334+
shouldUpdate = instance.shouldComponentUpdate(
335+
newProps,
336+
newState,
337+
nextContext,
338+
);
335339
} finally {
336340
reenableLogs();
337341
}
338342
}
339-
}
340-
const shouldUpdate = instance.shouldComponentUpdate(
341-
newProps,
342-
newState,
343-
nextContext,
344-
);
345-
346-
if (__DEV__) {
347343
if (shouldUpdate === undefined) {
348344
console.error(
349345
'%s.shouldComponentUpdate(): Returned undefined instead of a ' +
@@ -659,6 +655,7 @@ function constructClassInstance(
659655
: emptyContextObject;
660656
}
661657

658+
let instance = new ctor(props, context);
662659
// Instantiate twice to help detect side-effects.
663660
if (__DEV__) {
664661
if (
@@ -667,14 +664,13 @@ function constructClassInstance(
667664
) {
668665
disableLogs();
669666
try {
670-
new ctor(props, context); // eslint-disable-line no-new
667+
instance = new ctor(props, context); // eslint-disable-line no-new
671668
} finally {
672669
reenableLogs();
673670
}
674671
}
675672
}
676673

677-
const instance = new ctor(props, context);
678674
const state = (workInProgress.memoizedState =
679675
instance.state !== null && instance.state !== undefined
680676
? instance.state

‎packages/react-reconciler/src/ReactFiberClassComponent.old.js

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ function applyDerivedStateFromProps(
169169
nextProps: any,
170170
) {
171171
const prevState = workInProgress.memoizedState;
172-
172+
let partialState = getDerivedStateFromProps(nextProps, prevState);
173173
if (__DEV__) {
174174
if (
175175
debugRenderPhaseSideEffectsForStrictMode &&
@@ -178,16 +178,11 @@ function applyDerivedStateFromProps(
178178
disableLogs();
179179
try {
180180
// Invoke the function an extra time to help detect side-effects.
181-
getDerivedStateFromProps(nextProps, prevState);
181+
partialState = getDerivedStateFromProps(nextProps, prevState);
182182
} finally {
183183
reenableLogs();
184184
}
185185
}
186-
}
187-
188-
const partialState = getDerivedStateFromProps(nextProps, prevState);
189-
190-
if (__DEV__) {
191186
warnOnUndefinedDerivedState(ctor, partialState);
192187
}
193188
// Merge the partial state and the previous state.
@@ -323,6 +318,11 @@ function checkShouldComponentUpdate(
323318
) {
324319
const instance = workInProgress.stateNode;
325320
if (typeof instance.shouldComponentUpdate === 'function') {
321+
let shouldUpdate = instance.shouldComponentUpdate(
322+
newProps,
323+
newState,
324+
nextContext,
325+
);
326326
if (__DEV__) {
327327
if (
328328
debugRenderPhaseSideEffectsForStrictMode &&
@@ -331,19 +331,15 @@ function checkShouldComponentUpdate(
331331
disableLogs();
332332
try {
333333
// Invoke the function an extra time to help detect side-effects.
334-
instance.shouldComponentUpdate(newProps, newState, nextContext);
334+
shouldUpdate = instance.shouldComponentUpdate(
335+
newProps,
336+
newState,
337+
nextContext,
338+
);
335339
} finally {
336340
reenableLogs();
337341
}
338342
}
339-
}
340-
const shouldUpdate = instance.shouldComponentUpdate(
341-
newProps,
342-
newState,
343-
nextContext,
344-
);
345-
346-
if (__DEV__) {
347343
if (shouldUpdate === undefined) {
348344
console.error(
349345
'%s.shouldComponentUpdate(): Returned undefined instead of a ' +
@@ -659,6 +655,7 @@ function constructClassInstance(
659655
: emptyContextObject;
660656
}
661657

658+
let instance = new ctor(props, context);
662659
// Instantiate twice to help detect side-effects.
663660
if (__DEV__) {
664661
if (
@@ -667,14 +664,13 @@ function constructClassInstance(
667664
) {
668665
disableLogs();
669666
try {
670-
new ctor(props, context); // eslint-disable-line no-new
667+
instance = new ctor(props, context); // eslint-disable-line no-new
671668
} finally {
672669
reenableLogs();
673670
}
674671
}
675672
}
676673

677-
const instance = new ctor(props, context);
678674
const state = (workInProgress.memoizedState =
679675
instance.state !== null && instance.state !== undefined
680676
? instance.state

‎packages/react/src/__tests__/ReactStrictMode-test.js

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -892,4 +892,196 @@ describe('context legacy', () => {
892892
// Dedupe
893893
ReactDOM.render(<Root />, container);
894894
});
895+
896+
describe('disableLogs', () => {
897+
it('disables logs once for class double render', () => {
898+
spyOnDevAndProd(console, 'log');
899+
900+
let count = 0;
901+
class Foo extends React.Component {
902+
render() {
903+
count++;
904+
console.log('foo ' + count);
905+
return null;
906+
}
907+
}
908+
909+
const container = document.createElement('div');
910+
ReactDOM.render(
911+
<React.StrictMode>
912+
<Foo />
913+
</React.StrictMode>,
914+
container,
915+
);
916+
917+
expect(count).toBe(__DEV__ ? 2 : 1);
918+
expect(console.log).toBeCalledTimes(1);
919+
// Note: we should display the first log because otherwise
920+
// there is a risk of suppressing warnings when they happen,
921+
// and on the next render they'd get deduplicated and ignored.
922+
expect(console.log).toBeCalledWith('foo 1');
923+
});
924+
925+
it('disables logs once for class double ctor', () => {
926+
spyOnDevAndProd(console, 'log');
927+
928+
let count = 0;
929+
class Foo extends React.Component {
930+
constructor(props) {
931+
super(props);
932+
count++;
933+
console.log('foo ' + count);
934+
}
935+
render() {
936+
return null;
937+
}
938+
}
939+
940+
const container = document.createElement('div');
941+
ReactDOM.render(
942+
<React.StrictMode>
943+
<Foo />
944+
</React.StrictMode>,
945+
container,
946+
);
947+
948+
expect(count).toBe(__DEV__ ? 2 : 1);
949+
expect(console.log).toBeCalledTimes(1);
950+
// Note: we should display the first log because otherwise
951+
// there is a risk of suppressing warnings when they happen,
952+
// and on the next render they'd get deduplicated and ignored.
953+
expect(console.log).toBeCalledWith('foo 1');
954+
});
955+
956+
it('disables logs once for class double getDerivedStateFromProps', () => {
957+
spyOnDevAndProd(console, 'log');
958+
959+
let count = 0;
960+
class Foo extends React.Component {
961+
state = {};
962+
static getDerivedStateFromProps() {
963+
count++;
964+
console.log('foo ' + count);
965+
return {};
966+
}
967+
render() {
968+
return null;
969+
}
970+
}
971+
972+
const container = document.createElement('div');
973+
ReactDOM.render(
974+
<React.StrictMode>
975+
<Foo />
976+
</React.StrictMode>,
977+
container,
978+
);
979+
980+
expect(count).toBe(__DEV__ ? 2 : 1);
981+
expect(console.log).toBeCalledTimes(1);
982+
// Note: we should display the first log because otherwise
983+
// there is a risk of suppressing warnings when they happen,
984+
// and on the next render they'd get deduplicated and ignored.
985+
expect(console.log).toBeCalledWith('foo 1');
986+
});
987+
988+
it('disables logs once for class double shouldComponentUpdate', () => {
989+
spyOnDevAndProd(console, 'log');
990+
991+
let count = 0;
992+
class Foo extends React.Component {
993+
state = {};
994+
shouldComponentUpdate() {
995+
count++;
996+
console.log('foo ' + count);
997+
return {};
998+
}
999+
render() {
1000+
return null;
1001+
}
1002+
}
1003+
1004+
const container = document.createElement('div');
1005+
ReactDOM.render(
1006+
<React.StrictMode>
1007+
<Foo />
1008+
</React.StrictMode>,
1009+
container,
1010+
);
1011+
// Trigger sCU:
1012+
ReactDOM.render(
1013+
<React.StrictMode>
1014+
<Foo />
1015+
</React.StrictMode>,
1016+
container,
1017+
);
1018+
1019+
expect(count).toBe(__DEV__ ? 2 : 1);
1020+
expect(console.log).toBeCalledTimes(1);
1021+
// Note: we should display the first log because otherwise
1022+
// there is a risk of suppressing warnings when they happen,
1023+
// and on the next render they'd get deduplicated and ignored.
1024+
expect(console.log).toBeCalledWith('foo 1');
1025+
});
1026+
1027+
it('disables logs once for class state updaters', () => {
1028+
spyOnDevAndProd(console, 'log');
1029+
1030+
let inst;
1031+
let count = 0;
1032+
class Foo extends React.Component {
1033+
state = {};
1034+
render() {
1035+
inst = this;
1036+
return null;
1037+
}
1038+
}
1039+
1040+
const container = document.createElement('div');
1041+
ReactDOM.render(
1042+
<React.StrictMode>
1043+
<Foo />
1044+
</React.StrictMode>,
1045+
container,
1046+
);
1047+
inst.setState(() => {
1048+
count++;
1049+
console.log('foo ' + count);
1050+
return {};
1051+
});
1052+
1053+
expect(count).toBe(__DEV__ ? 2 : 1);
1054+
expect(console.log).toBeCalledTimes(1);
1055+
// Note: we should display the first log because otherwise
1056+
// there is a risk of suppressing warnings when they happen,
1057+
// and on the next render they'd get deduplicated and ignored.
1058+
expect(console.log).toBeCalledWith('foo 1');
1059+
});
1060+
1061+
it('disables logs once for function double render', () => {
1062+
spyOnDevAndProd(console, 'log');
1063+
1064+
let count = 0;
1065+
function Foo() {
1066+
count++;
1067+
console.log('foo ' + count);
1068+
return null;
1069+
}
1070+
1071+
const container = document.createElement('div');
1072+
ReactDOM.render(
1073+
<React.StrictMode>
1074+
<Foo />
1075+
</React.StrictMode>,
1076+
container,
1077+
);
1078+
1079+
expect(count).toBe(__DEV__ ? 2 : 1);
1080+
expect(console.log).toBeCalledTimes(1);
1081+
// Note: we should display the first log because otherwise
1082+
// there is a risk of suppressing warnings when they happen,
1083+
// and on the next render they'd get deduplicated and ignored.
1084+
expect(console.log).toBeCalledWith('foo 1');
1085+
});
1086+
});
8951087
});

0 commit comments

Comments
 (0)