Don't dedupe using the stack (#18693)
We currently use the stack to dedupe warnings in a couple of places. This is a very heavy weight way of computing that a warning doesn't need to be fired. This uses parent component name as a heuristic for deduping. It's not perfect but as soon as you fix one you'll uncover the next. It might be a little annoying but having many logs is also annoying. We now have no special cases for stacks. The only thing that uses stacks in dev is the console.error and dev tools. This means that we could externalize this completely to an console.error patching module and drop it from being built-in to react. The only prod/dev behavior is the one we pass to error boundaries or the error we throw if you don't have an error boundary.
Sebastian Markbåge committed
Apr 22, 2020 at 19:02 UTC
b0cb137bcbd3a11d8eff3c2229cd6b8379d29785
8 files changed
+115
-108
packages/react-dom/src/__tests__/ReactComponent-test.js
+2
@@ -16,6 +16,8 @@ let ReactTestUtils;
16
17
describe('ReactComponent', () => {
18
beforeEach(() => {
19
+ jest.resetModules();
20
+
21
React = require('react');
22
ReactDOM = require('react-dom');
23
ReactDOMServer = require('react-dom/server');
packages/react-dom/src/__tests__/ReactDOMComponent-test.js
+70
-32
@@ -1689,26 +1689,12 @@ describe('ReactDOMComponent', () => {
1689
<tr />
1690
</div>,
1691
);
1692
- }).toErrorDev(
1693
- ReactFeatureFlags.enableComponentStackLocations
1694
- ? [
1695
- // This warning dedupes since they're in the same component.
1696
- 'Warning: validateDOMNesting(...): <tr> cannot appear as a child of ' +
1697
- '<div>.' +
1698
- '\n in tr (at **)' +
1699
- '\n in div (at **)',
1700
- ]
1701
- : [
1702
- 'Warning: validateDOMNesting(...): <tr> cannot appear as a child of ' +
1703
- '<div>.' +
1704
- '\n in tr (at **)' +
1705
- '\n in div (at **)',
1706
- 'Warning: validateDOMNesting(...): <tr> cannot appear as a child of ' +
1707
- '<div>.' +
1708
- '\n in tr (at **)' +
1709
- '\n in div (at **)',
1710
- ],
1711
- );
1692
+ }).toErrorDev([
1693
+ 'Warning: validateDOMNesting(...): <tr> cannot appear as a child of ' +
1694
+ '<div>.' +
1695
+ '\n in tr (at **)' +
1696
+ '\n in div (at **)',
1697
+ ]);
1698
});
1699
1700
it('warns on invalid nesting at root', () => {
@@ -1777,18 +1763,6 @@ describe('ReactDOMComponent', () => {
1763
return <Row />;
1764
}
1765
1780
- class Table extends React.Component {
1781
- render() {
1782
- return <table>{this.props.children}</table>;
1783
- }
1784
- }
1785
-
1786
- class FancyTable extends React.Component {
1787
- render() {
1788
- return <Table>{this.props.children}</Table>;
1789
- }
1790
- }
1791
-
1766
function Viz1() {
1767
return (
1768
<table>
@@ -1806,6 +1780,27 @@ describe('ReactDOMComponent', () => {
1780
'\n in table (at **)' +
1781
'\n in Viz1 (at **)',
1782
);
1783
+ });
1784
+
1785
+ it('gives useful context in warnings 2', () => {
1786
+ function Row() {
1787
+ return <tr />;
1788
+ }
1789
+ function FancyRow() {
1790
+ return <Row />;
1791
+ }
1792
+
1793
+ class Table extends React.Component {
1794
+ render() {
1795
+ return <table>{this.props.children}</table>;
1796
+ }
1797
+ }
1798
+
1799
+ class FancyTable extends React.Component {
1800
+ render() {
1801
+ return <Table>{this.props.children}</Table>;
1802
+ }
1803
+ }
1804
1805
function Viz2() {
1806
return (
@@ -1826,7 +1821,27 @@ describe('ReactDOMComponent', () => {
1821
'\n in FancyTable (at **)' +
1822
'\n in Viz2 (at **)',
1823
);
1824
+ });
1825
1826
+ it('gives useful context in warnings 3', () => {
1827
+ function Row() {
1828
+ return <tr />;
1829
+ }
1830
+ function FancyRow() {
1831
+ return <Row />;
1832
+ }
1833
+
1834
+ class Table extends React.Component {
1835
+ render() {
1836
+ return <table>{this.props.children}</table>;
1837
+ }
1838
+ }
1839
+
1840
+ class FancyTable extends React.Component {
1841
+ render() {
1842
+ return <Table>{this.props.children}</Table>;
1843
+ }
1844
+ }
1845
expect(() => {
1846
ReactTestUtils.renderIntoDocument(
1847
<FancyTable>
@@ -1841,6 +1856,15 @@ describe('ReactDOMComponent', () => {
1856
'\n in Table (at **)' +
1857
'\n in FancyTable (at **)',
1858
);
1859
+ });
1860
+
1861
+ it('gives useful context in warnings 4', () => {
1862
+ function Row() {
1863
+ return <tr />;
1864
+ }
1865
+ function FancyRow() {
1866
+ return <Row />;
1867
+ }
1868
1869
expect(() => {
1870
ReactTestUtils.renderIntoDocument(
@@ -1854,6 +1878,20 @@ describe('ReactDOMComponent', () => {
1878
'\n in FancyRow (at **)' +
1879
'\n in table (at **)',
1880
);
1881
+ });
1882
+
1883
+ it('gives useful context in warnings 5', () => {
1884
+ class Table extends React.Component {
1885
+ render() {
1886
+ return <table>{this.props.children}</table>;
1887
+ }
1888
+ }
1889
+
1890
+ class FancyTable extends React.Component {
1891
+ render() {
1892
+ return <Table>{this.props.children}</Table>;
1893
+ }
1894
+ }
1895
1896
expect(() => {
1897
ReactTestUtils.renderIntoDocument(
packages/react-dom/src/client/validateDOMNesting.js
+1
-6
@@ -5,9 +5,6 @@
5
* LICENSE file in the root directory of this source tree.
6
*/
7
8
-// TODO: direct imports like some-package/src/* are bad. Fix me.
9
-import {getCurrentFiberStackInDev} from 'react-reconciler/src/ReactCurrentFiber';
10
-
8
let validateDOMNesting = () => {};
9
let updatedAncestorInfo = () => {};
10
@@ -430,10 +427,8 @@ if (__DEV__) {
427
}
428
429
const ancestorTag = invalidParentOrAncestor.tag;
433
- const addendum = getCurrentFiberStackInDev();
430
435
- const warnKey =
436
- !!invalidParent + '|' + childTag + '|' + ancestorTag + '|' + addendum;
431
+ const warnKey = !!invalidParent + '|' + childTag + '|' + ancestorTag;
432
if (didWarn[warnKey]) {
433
return;
434
}
packages/react-reconciler/src/ReactChildFiber.new.js
+18
-25
@@ -44,7 +44,6 @@ import {
44
createFiberFromPortal,
45
} from './ReactFiber.new';
46
import {emptyRefsObject} from './ReactFiberClassComponent.new';
47
-import {getCurrentFiberStackInDev} from './ReactCurrentFiber';
47
import {isCompatibleFamilyForHotReloading} from './ReactFiberHotReloading.new';
48
import {StrictMode} from './ReactTypeOfMode';
49
@@ -53,7 +52,7 @@ let didWarnAboutGenerators;
52
let didWarnAboutStringRefs;
53
let ownerHasKeyUseWarning;
54
let ownerHasFunctionTypeWarning;
56
-let warnForMissingKey = (child: mixed) => {};
55
+let warnForMissingKey = (child: mixed, returnFiber: Fiber) => {};
56
57
if (__DEV__) {
58
didWarnAboutMaps = false;
@@ -68,7 +67,7 @@ if (__DEV__) {
67
ownerHasKeyUseWarning = {};
68
ownerHasFunctionTypeWarning = {};
69
71
- warnForMissingKey = (child: mixed) => {
70
+ warnForMissingKey = (child: mixed, returnFiber: Fiber) => {
71
if (child === null || typeof child !== 'object') {
72
return;
73
}
@@ -82,15 +81,12 @@ if (__DEV__) {
81
);
82
child._store.validated = true;
83
85
- const currentComponentErrorInfo =
86
- 'Each child in a list should have a unique ' +
87
- '"key" prop. See https://fb.me/react-warning-keys for ' +
88
- 'more information.' +
89
- getCurrentFiberStackInDev();
90
- if (ownerHasKeyUseWarning[currentComponentErrorInfo]) {
84
+ const componentName = getComponentName(returnFiber.type) || 'Component';
85
+
86
+ if (ownerHasKeyUseWarning[componentName]) {
87
return;
88
}
93
- ownerHasKeyUseWarning[currentComponentErrorInfo] = true;
89
+ ownerHasKeyUseWarning[componentName] = true;
90
91
console.error(
92
'Each child in a list should have a unique ' +
@@ -232,18 +228,14 @@ function throwOnInvalidObjectType(returnFiber: Fiber, newChild: Object) {
228
}
229
}
230
235
-function warnOnFunctionType() {
231
+function warnOnFunctionType(returnFiber: Fiber) {
232
if (__DEV__) {
237
- const currentComponentErrorInfo =
238
- 'Functions are not valid as a React child. This may happen if ' +
239
- 'you return a Component instead of <Component /> from render. ' +
240
- 'Or maybe you meant to call this function rather than return it.' +
241
- getCurrentFiberStackInDev();
233
+ const componentName = getComponentName(returnFiber.type) || 'Component';
234
243
- if (ownerHasFunctionTypeWarning[currentComponentErrorInfo]) {
235
+ if (ownerHasFunctionTypeWarning[componentName]) {
236
return;
237
}
246
- ownerHasFunctionTypeWarning[currentComponentErrorInfo] = true;
238
+ ownerHasFunctionTypeWarning[componentName] = true;
239
240
console.error(
241
'Functions are not valid as a React child. This may happen if ' +
@@ -570,7 +562,7 @@ function ChildReconciler(shouldTrackSideEffects) {
562
563
if (__DEV__) {
564
if (typeof newChild === 'function') {
573
- warnOnFunctionType();
565
+ warnOnFunctionType(returnFiber);
566
}
567
}
568
@@ -658,7 +650,7 @@ function ChildReconciler(shouldTrackSideEffects) {
650
651
if (__DEV__) {
652
if (typeof newChild === 'function') {
661
- warnOnFunctionType();
653
+ warnOnFunctionType(returnFiber);
654
}
655
}
656
@@ -737,7 +729,7 @@ function ChildReconciler(shouldTrackSideEffects) {
729
730
if (__DEV__) {
731
if (typeof newChild === 'function') {
740
- warnOnFunctionType();
732
+ warnOnFunctionType(returnFiber);
733
}
734
}
735
@@ -750,6 +742,7 @@ function ChildReconciler(shouldTrackSideEffects) {
742
function warnOnInvalidKey(
743
child: mixed,
744
knownKeys: Set<string> | null,
745
+ returnFiber: Fiber,
746
): Set<string> | null {
747
if (__DEV__) {
748
if (typeof child !== 'object' || child === null) {
@@ -758,7 +751,7 @@ function ChildReconciler(shouldTrackSideEffects) {
751
switch (child.$$typeof) {
752
case REACT_ELEMENT_TYPE:
753
case REACT_PORTAL_TYPE:
761
- warnForMissingKey(child);
754
+ warnForMissingKey(child, returnFiber);
755
const key = child.key;
756
if (typeof key !== 'string') {
757
break;
@@ -818,7 +811,7 @@ function ChildReconciler(shouldTrackSideEffects) {
811
let knownKeys = null;
812
for (let i = 0; i < newChildren.length; i++) {
813
const child = newChildren[i];
821
- knownKeys = warnOnInvalidKey(child, knownKeys);
814
+ knownKeys = warnOnInvalidKey(child, knownKeys, returnFiber);
815
}
816
}
817
@@ -1002,7 +995,7 @@ function ChildReconciler(shouldTrackSideEffects) {
995
let step = newChildren.next();
996
for (; !step.done; step = newChildren.next()) {
997
const child = step.value;
1005
- knownKeys = warnOnInvalidKey(child, knownKeys);
998
+ knownKeys = warnOnInvalidKey(child, knownKeys, returnFiber);
999
}
1000
}
1001
}
@@ -1396,7 +1389,7 @@ function ChildReconciler(shouldTrackSideEffects) {
1389
1390
if (__DEV__) {
1391
if (typeof newChild === 'function') {
1399
- warnOnFunctionType();
1392
+ warnOnFunctionType(returnFiber);
1393
}
1394
}
1395
if (typeof newChild === 'undefined' && !isUnkeyedTopLevelFragment) {
packages/react-reconciler/src/ReactChildFiber.old.js
+18
-26
@@ -44,7 +44,6 @@ import {
44
createFiberFromPortal,
45
} from './ReactFiber.old';
46
import {emptyRefsObject} from './ReactFiberClassComponent.old';
47
-import {getCurrentFiberStackInDev} from './ReactCurrentFiber';
47
import {isCompatibleFamilyForHotReloading} from './ReactFiberHotReloading.old';
48
import {StrictMode} from './ReactTypeOfMode';
49
@@ -53,7 +52,7 @@ let didWarnAboutGenerators;
52
let didWarnAboutStringRefs;
53
let ownerHasKeyUseWarning;
54
let ownerHasFunctionTypeWarning;
56
-let warnForMissingKey = (child: mixed) => {};
55
+let warnForMissingKey = (child: mixed, returnFiber: Fiber) => {};
56
57
if (__DEV__) {
58
didWarnAboutMaps = false;
@@ -68,7 +67,7 @@ if (__DEV__) {
67
ownerHasKeyUseWarning = {};
68
ownerHasFunctionTypeWarning = {};
69
71
- warnForMissingKey = (child: mixed) => {
70
+ warnForMissingKey = (child: mixed, returnFiber: Fiber) => {
71
if (child === null || typeof child !== 'object') {
72
return;
73
}
@@ -82,15 +81,12 @@ if (__DEV__) {
81
);
82
child._store.validated = true;
83
85
- const currentComponentErrorInfo =
86
- 'Each child in a list should have a unique ' +
87
- '"key" prop. See https://fb.me/react-warning-keys for ' +
88
- 'more information.' +
89
- getCurrentFiberStackInDev();
90
- if (ownerHasKeyUseWarning[currentComponentErrorInfo]) {
84
+ const componentName = getComponentName(returnFiber.type) || 'Component';
85
+
86
+ if (ownerHasKeyUseWarning[componentName]) {
87
return;
88
}
93
- ownerHasKeyUseWarning[currentComponentErrorInfo] = true;
89
+ ownerHasKeyUseWarning[componentName] = true;
90
91
console.error(
92
'Each child in a list should have a unique ' +
@@ -231,18 +227,13 @@ function throwOnInvalidObjectType(returnFiber: Fiber, newChild: Object) {
227
}
228
}
229
234
-function warnOnFunctionType() {
230
+function warnOnFunctionType(returnFiber: Fiber) {
231
if (__DEV__) {
236
- const currentComponentErrorInfo =
237
- 'Functions are not valid as a React child. This may happen if ' +
238
- 'you return a Component instead of <Component /> from render. ' +
239
- 'Or maybe you meant to call this function rather than return it.' +
240
- getCurrentFiberStackInDev();
241
-
242
- if (ownerHasFunctionTypeWarning[currentComponentErrorInfo]) {
232
+ const componentName = getComponentName(returnFiber.type) || 'Component';
233
+ if (ownerHasFunctionTypeWarning[componentName]) {
234
return;
235
}
245
- ownerHasFunctionTypeWarning[currentComponentErrorInfo] = true;
236
+ ownerHasFunctionTypeWarning[componentName] = true;
237
238
console.error(
239
'Functions are not valid as a React child. This may happen if ' +
@@ -569,7 +560,7 @@ function ChildReconciler(shouldTrackSideEffects) {
560
561
if (__DEV__) {
562
if (typeof newChild === 'function') {
572
- warnOnFunctionType();
563
+ warnOnFunctionType(returnFiber);
564
}
565
}
566
@@ -657,7 +648,7 @@ function ChildReconciler(shouldTrackSideEffects) {
648
649
if (__DEV__) {
650
if (typeof newChild === 'function') {
660
- warnOnFunctionType();
651
+ warnOnFunctionType(returnFiber);
652
}
653
}
654
@@ -736,7 +727,7 @@ function ChildReconciler(shouldTrackSideEffects) {
727
728
if (__DEV__) {
729
if (typeof newChild === 'function') {
739
- warnOnFunctionType();
730
+ warnOnFunctionType(returnFiber);
731
}
732
}
733
@@ -749,6 +740,7 @@ function ChildReconciler(shouldTrackSideEffects) {
740
function warnOnInvalidKey(
741
child: mixed,
742
knownKeys: Set<string> | null,
743
+ returnFiber: Fiber,
744
): Set<string> | null {
745
if (__DEV__) {
746
if (typeof child !== 'object' || child === null) {
@@ -757,7 +749,7 @@ function ChildReconciler(shouldTrackSideEffects) {
749
switch (child.$$typeof) {
750
case REACT_ELEMENT_TYPE:
751
case REACT_PORTAL_TYPE:
760
- warnForMissingKey(child);
752
+ warnForMissingKey(child, returnFiber);
753
const key = child.key;
754
if (typeof key !== 'string') {
755
break;
@@ -817,7 +809,7 @@ function ChildReconciler(shouldTrackSideEffects) {
809
let knownKeys = null;
810
for (let i = 0; i < newChildren.length; i++) {
811
const child = newChildren[i];
820
- knownKeys = warnOnInvalidKey(child, knownKeys);
812
+ knownKeys = warnOnInvalidKey(child, knownKeys, returnFiber);
813
}
814
}
815
@@ -1001,7 +993,7 @@ function ChildReconciler(shouldTrackSideEffects) {
993
let step = newChildren.next();
994
for (; !step.done; step = newChildren.next()) {
995
const child = step.value;
1004
- knownKeys = warnOnInvalidKey(child, knownKeys);
996
+ knownKeys = warnOnInvalidKey(child, knownKeys, returnFiber);
997
}
998
}
999
}
@@ -1395,7 +1387,7 @@ function ChildReconciler(shouldTrackSideEffects) {
1387
1388
if (__DEV__) {
1389
if (typeof newChild === 'function') {
1398
- warnOnFunctionType();
1390
+ warnOnFunctionType(returnFiber);
1391
}
1392
}
1393
if (typeof newChild === 'undefined' && !isUnkeyedTopLevelFragment) {
packages/react-reconciler/src/ReactCurrentFiber.js
+1
-1
@@ -31,7 +31,7 @@ export function getCurrentFiberOwnerNameInDevOrNull(): string | null {
31
return null;
32
}
33
34
-export function getCurrentFiberStackInDev(): string {
34
+function getCurrentFiberStackInDev(): string {
35
if (__DEV__) {
36
if (current === null) {
37
return '';
packages/react-reconciler/src/__tests__/ReactFragment-test.js
+4
-18
@@ -10,7 +10,6 @@
10
'use strict';
11
12
let React;
13
-let ReactFeatureFlags;
13
let ReactNoop;
14
let Scheduler;
15
@@ -19,7 +18,6 @@ describe('ReactFragment', () => {
18
jest.resetModules();
19
20
React = require('react');
22
- ReactFeatureFlags = require('shared/ReactFeatureFlags');
21
ReactNoop = require('react-noop-renderer');
22
Scheduler = require('scheduler');
23
});
@@ -902,27 +900,15 @@ describe('ReactFragment', () => {
900
);
901
902
ReactNoop.render(<Foo condition={false} />);
905
- if (ReactFeatureFlags.enableComponentStackLocations) {
906
- // The key warning gets deduped because it's in the same component.
907
- expect(Scheduler).toFlushWithoutYielding();
908
- } else {
909
- expect(() => expect(Scheduler).toFlushWithoutYielding()).toErrorDev(
910
- 'Each child in a list should have a unique "key" prop.',
911
- );
912
- }
903
+ // The key warning gets deduped because it's in the same component.
904
+ expect(Scheduler).toFlushWithoutYielding();
905
906
expect(ops).toEqual(['Update Stateful']);
907
expect(ReactNoop.getChildren()).toEqual([span(), div()]);
908
909
ReactNoop.render(<Foo condition={true} />);
918
- if (ReactFeatureFlags.enableComponentStackLocations) {
919
- // The key warning gets deduped because it's in the same component.
920
- expect(Scheduler).toFlushWithoutYielding();
921
- } else {
922
- expect(() => expect(Scheduler).toFlushWithoutYielding()).toErrorDev(
923
- 'Each child in a list should have a unique "key" prop.',
924
- );
925
- }
910
+ // The key warning gets deduped because it's in the same component.
911
+ expect(Scheduler).toFlushWithoutYielding();
912
913
expect(ops).toEqual(['Update Stateful', 'Update Stateful']);
914
expect(ReactNoop.getChildren()).toEqual([span(), div()]);
packages/shared/__tests__/describeComponentFrame-test.js
+1
@@ -89,6 +89,7 @@ describe('Component stack trace displaying', () => {
89
'C:\\funny long (path)/index.jsx': 'funny long (path)/index.jsx',
90
};
91
Object.keys(fileNames).forEach((fileName, i) => {
92
+ Component.displayName = 'Component ' + i;
93
ReactDOM.render(
94
<Component __source={{fileName, lineNumber: i}} />,
95
container,