Skip to content

Commit d6cfa0f

Browse files
authored
[Fiber] Use Owner/JSX Stack When Appending Stacks to Console (#29206)
This one should be fully behind the `enableOwnerStacks` flag. Instead of printing the parent Component stack all the way to the root, this now prints the owner stack of every JSX callsite. It also includes intermediate callsites between the Component and the JSX call so it has potentially more frames. Mainly it provides the line number of the JSX callsite. In terms of the number of components is a subset of the parent component stack so it's less information in that regard. This is usually better since it's more focused on components that might affect the output but if it's contextual based on rendering it's still good to have parent stack. Therefore, I still use the parent stack when printing DOM nesting warnings but I plan on switching that format to a diff view format instead (Next.js already reformats the parent stack like this). __Follow ups__ - Server Components show up in the owner stack for client logs but logs done by Server Components don't yet get their owner stack printed as they're replayed. They're also not yet printed in the server logs of the RSC server. - Server Component stack frames are formatted as the server and added to the end but this might be a different format than the browser. E.g. if server is running V8 and browser is running JSC or vice versa. Ideally we can reformat them in terms of the client formatting. - This doesn't yet update Fizz or DevTools. Those will be follow ups. Fizz still prints parent stacks in the server side logs. The stacks added to user space `console.error` calls by DevTools still get the parent stacks instead. - It also doesn't yet expose these to user space so there's no way to get them inside `onCaughtError` for example or inside a custom `console.error` override. - In another follow up I'll use `console.createTask` instead and completely remove these stacks if it's available.
1 parent 935180c commit d6cfa0f

34 files changed

+591
-145
lines changed

.eslintrc.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -486,6 +486,7 @@ module.exports = {
486486
$ReadOnlyArray: 'readonly',
487487
$ArrayBufferView: 'readonly',
488488
$Shape: 'readonly',
489+
ConsoleTask: 'readonly', // TOOD: Figure out what the official name of this will be.
489490
ReturnType: 'readonly',
490491
AnimationFrameID: 'readonly',
491492
// For Flow type annotation. Only `BigInt` is valid at runtime.

packages/react-client/src/__tests__/ReactFlight-test.js

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1123,10 +1123,11 @@ describe('ReactFlight', () => {
11231123
}
11241124

11251125
function App() {
1126-
return (
1127-
<Indirection>
1128-
<ClientComponent />
1129-
</Indirection>
1126+
// We use the ReactServer runtime here to get the Server owner.
1127+
return ReactServer.createElement(
1128+
Indirection,
1129+
null,
1130+
ReactServer.createElement(ClientComponent),
11301131
);
11311132
}
11321133

@@ -1143,11 +1144,10 @@ describe('ReactFlight', () => {
11431144
'\n' +
11441145
'Check the render method of `Component`. See https://react.dev/link/warning-keys for more information.\n' +
11451146
' in span (at **)\n' +
1146-
// TODO: Because this validates after the div has been mounted, it is part of
1147-
// the parent stack but since owner stacks will switch to owners this goes away again.
1148-
(gate(flags => flags.enableOwnerStacks) ? ' in div (at **)\n' : '') +
11491147
' in Component (at **)\n' +
1150-
' in Indirection (at **)\n' +
1148+
(gate(flags => flags.enableOwnerStacks)
1149+
? ''
1150+
: ' in Indirection (at **)\n') +
11511151
' in App (at **)',
11521152
);
11531153
});

packages/react-dom-bindings/src/client/validateDOMNesting.js

Lines changed: 38 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
* @flow
88
*/
99

10+
import {getCurrentParentStackInDev} from 'react-reconciler/src/ReactCurrentFiber';
11+
1012
type Info = {tag: string};
1113
export type AncestorInfoDev = {
1214
current: ?Info,
@@ -476,19 +478,31 @@ function validateDOMNesting(
476478
' Add a <tbody>, <thead> or <tfoot> to your code to match the DOM tree generated by ' +
477479
'the browser.';
478480
}
479-
console.error(
480-
'In HTML, %s cannot be a child of <%s>.%s\n' +
481-
'This will cause a hydration error.',
481+
// Don't transform into consoleWithStackDev here because we add a manual stack.
482+
// We use the parent stack here instead of the owner stack because the parent
483+
// stack has more useful context for nesting.
484+
// TODO: Format this as a linkified "diff view" with props instead of
485+
// a stack trace since the stack trace format is now for owner stacks.
486+
console['error'](
487+
'Warning: In HTML, %s cannot be a child of <%s>.%s\n' +
488+
'This will cause a hydration error.%s',
482489
tagDisplayName,
483490
ancestorTag,
484491
info,
492+
getCurrentParentStackInDev(),
485493
);
486494
} else {
487-
console.error(
488-
'In HTML, %s cannot be a descendant of <%s>.\n' +
489-
'This will cause a hydration error.',
495+
// Don't transform into consoleWithStackDev here because we add a manual stack.
496+
// We use the parent stack here instead of the owner stack because the parent
497+
// stack has more useful context for nesting.
498+
// TODO: Format this as a linkified "diff view" with props instead of
499+
// a stack trace since the stack trace format is now for owner stacks.
500+
console['error'](
501+
'Warning: In HTML, %s cannot be a descendant of <%s>.\n' +
502+
'This will cause a hydration error.%s',
490503
tagDisplayName,
491504
ancestorTag,
505+
getCurrentParentStackInDev(),
492506
);
493507
}
494508
return false;
@@ -510,18 +524,30 @@ function validateTextNesting(childText: string, parentTag: string): boolean {
510524
didWarn[warnKey] = true;
511525

512526
if (/\S/.test(childText)) {
513-
console.error(
514-
'In HTML, text nodes cannot be a child of <%s>.\n' +
515-
'This will cause a hydration error.',
527+
// Don't transform into consoleWithStackDev here because we add a manual stack.
528+
// We use the parent stack here instead of the owner stack because the parent
529+
// stack has more useful context for nesting.
530+
// TODO: Format this as a linkified "diff view" with props instead of
531+
// a stack trace since the stack trace format is now for owner stacks.
532+
console['error'](
533+
'Warning: In HTML, text nodes cannot be a child of <%s>.\n' +
534+
'This will cause a hydration error.%s',
516535
parentTag,
536+
getCurrentParentStackInDev(),
517537
);
518538
} else {
519-
console.error(
520-
'In HTML, whitespace text nodes cannot be a child of <%s>. ' +
539+
// Don't transform into consoleWithStackDev here because we add a manual stack.
540+
// We use the parent stack here instead of the owner stack because the parent
541+
// stack has more useful context for nesting.
542+
// TODO: Format this as a linkified "diff view" with props instead of
543+
// a stack trace since the stack trace format is now for owner stacks.
544+
console['error'](
545+
'Warning: In HTML, whitespace text nodes cannot be a child of <%s>. ' +
521546
"Make sure you don't have any extra whitespace between tags on " +
522547
'each line of your source code.\n' +
523-
'This will cause a hydration error.',
548+
'This will cause a hydration error.%s',
524549
parentTag,
550+
getCurrentParentStackInDev(),
525551
);
526552
}
527553
return false;

packages/react-dom/src/__tests__/ReactChildReconciler-test.js

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,9 @@ describe('ReactChildReconciler', () => {
130130
'could change in a future version.\n' +
131131
' in div (at **)\n' +
132132
' in Component (at **)\n' +
133-
' in Parent (at **)\n' +
133+
(gate(flags => flags.enableOwnerStacks)
134+
? ''
135+
: ' in Parent (at **)\n') +
134136
' in GrandParent (at **)',
135137
);
136138
});
@@ -189,7 +191,9 @@ describe('ReactChildReconciler', () => {
189191
'could change in a future version.\n' +
190192
' in div (at **)\n' +
191193
' in Component (at **)\n' +
192-
' in Parent (at **)\n' +
194+
(gate(flags => flags.enableOwnerStacks)
195+
? ''
196+
: ' in Parent (at **)\n') +
193197
' in GrandParent (at **)',
194198
);
195199
});

packages/react-dom/src/__tests__/ReactComponent-test.js

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -761,7 +761,9 @@ describe('ReactComponent', () => {
761761
'Or maybe you meant to call this function rather than return it.\n' +
762762
' <span>{Foo}</span>\n' +
763763
' in span (at **)\n' +
764-
' in div (at **)\n' +
764+
(gate(flags => flags.enableOwnerStacks)
765+
? ''
766+
: ' in div (at **)\n') +
765767
' in Foo (at **)',
766768
);
767769
});
@@ -820,7 +822,9 @@ describe('ReactComponent', () => {
820822
'Or maybe you meant to call this function rather than return it.\n' +
821823
' <span>{Foo}</span>\n' +
822824
' in span (at **)\n' +
823-
' in div (at **)\n' +
825+
(gate(flags => flags.enableOwnerStacks)
826+
? ''
827+
: ' in div (at **)\n') +
824828
' in Foo (at **)',
825829
]);
826830
await act(() => {

packages/react-dom/src/__tests__/ReactDOM-test.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -552,7 +552,7 @@ describe('ReactDOM', () => {
552552
// ReactDOM(App > div > span)
553553
'Invalid ARIA attribute `ariaTypo`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
554554
' in span (at **)\n' +
555-
' in div (at **)\n' +
555+
(gate(flags => flags.enableOwnerStacks) ? '' : ' in div (at **)\n') +
556556
' in App (at **)',
557557
// ReactDOM(App > div > ServerEntry) >>> ReactDOMServer(Child) >>> ReactDOMServer(App2) >>> ReactDOMServer(blink)
558558
'Invalid ARIA attribute `ariaTypo2`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
@@ -569,7 +569,7 @@ describe('ReactDOM', () => {
569569
// ReactDOM(App > div > font)
570570
'Invalid ARIA attribute `ariaTypo5`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
571571
' in font (at **)\n' +
572-
' in div (at **)\n' +
572+
(gate(flags => flags.enableOwnerStacks) ? '' : ' in div (at **)\n') +
573573
' in App (at **)',
574574
]);
575575
});

packages/react-dom/src/__tests__/ReactMultiChild-test.js

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,9 @@ describe('ReactMultiChild', () => {
229229
'could change in a future version.\n' +
230230
' in div (at **)\n' +
231231
' in WrapperComponent (at **)\n' +
232-
' in div (at **)\n' +
232+
(gate(flags => flags.enableOwnerStacks)
233+
? ''
234+
: ' in div (at **)\n') +
233235
' in Parent (at **)',
234236
);
235237
});
@@ -292,7 +294,9 @@ describe('ReactMultiChild', () => {
292294
'could change in a future version.\n' +
293295
' in div (at **)\n' +
294296
' in WrapperComponent (at **)\n' +
295-
' in div (at **)\n' +
297+
(gate(flags => flags.enableOwnerStacks)
298+
? ''
299+
: ' in div (at **)\n') +
296300
' in Parent (at **)',
297301
);
298302
});

packages/react-dom/src/__tests__/ReactUpdates-test.js

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1848,7 +1848,7 @@ describe('ReactUpdates', () => {
18481848
it('warns about a deferred infinite update loop with useEffect', async () => {
18491849
function NonTerminating() {
18501850
const [step, setStep] = React.useState(0);
1851-
React.useEffect(() => {
1851+
React.useEffect(function myEffect() {
18521852
setStep(x => x + 1);
18531853
});
18541854
return step;
@@ -1860,10 +1860,12 @@ describe('ReactUpdates', () => {
18601860

18611861
let error = null;
18621862
let stack = null;
1863+
let nativeStack = null;
18631864
const originalConsoleError = console.error;
18641865
console.error = (e, s) => {
18651866
error = e;
18661867
stack = s;
1868+
nativeStack = new Error().stack;
18671869
Scheduler.log('stop');
18681870
};
18691871
try {
@@ -1876,7 +1878,14 @@ describe('ReactUpdates', () => {
18761878
}
18771879

18781880
expect(error).toContain('Maximum update depth exceeded');
1879-
expect(stack).toContain('at NonTerminating');
1881+
// The currently executing effect should be on the native stack
1882+
expect(nativeStack).toContain('at myEffect');
1883+
if (!gate(flags => flags.enableOwnerStacks)) {
1884+
// The currently running component's name is not in the owner
1885+
// stack because it's just its JSX callsite.
1886+
expect(stack).toContain('at NonTerminating');
1887+
}
1888+
expect(stack).toContain('at App');
18801889
});
18811890

18821891
it('can have nested updates if they do not cross the limit', async () => {

packages/react-reconciler/src/ReactChildFiber.js

Lines changed: 44 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ import {getIsHydrating} from './ReactFiberHydrationContext';
6161
import {pushTreeFork} from './ReactFiberTreeContext';
6262
import {createThenableState, trackUsedThenable} from './ReactFiberThenable';
6363
import {readContextDuringReconciliation} from './ReactFiberNewContext';
64+
import {callLazyInitInDEV} from './ReactFiberCallUserSpace';
6465

6566
import {
6667
getCurrentFiber as getCurrentDebugFiberInDEV,
@@ -362,6 +363,9 @@ function warnOnSymbolType(returnFiber: Fiber, invalidChild: symbol) {
362363
}
363364

364365
function resolveLazy(lazyType: any) {
366+
if (__DEV__) {
367+
return callLazyInitInDEV(lazyType);
368+
}
365369
const payload = lazyType._payload;
366370
const init = lazyType._init;
367371
return init(payload);
@@ -683,11 +687,17 @@ function createChildReconciler(
683687
return created;
684688
}
685689
case REACT_LAZY_TYPE: {
686-
const payload = newChild._payload;
687-
const init = newChild._init;
690+
let resolvedChild;
691+
if (__DEV__) {
692+
resolvedChild = callLazyInitInDEV(newChild);
693+
} else {
694+
const payload = newChild._payload;
695+
const init = newChild._init;
696+
resolvedChild = init(payload);
697+
}
688698
return createChild(
689699
returnFiber,
690-
init(payload),
700+
resolvedChild,
691701
lanes,
692702
mergeDebugInfo(debugInfo, newChild._debugInfo), // call merge after init
693703
);
@@ -811,12 +821,18 @@ function createChildReconciler(
811821
}
812822
}
813823
case REACT_LAZY_TYPE: {
814-
const payload = newChild._payload;
815-
const init = newChild._init;
824+
let resolvedChild;
825+
if (__DEV__) {
826+
resolvedChild = callLazyInitInDEV(newChild);
827+
} else {
828+
const payload = newChild._payload;
829+
const init = newChild._init;
830+
resolvedChild = init(payload);
831+
}
816832
return updateSlot(
817833
returnFiber,
818834
oldFiber,
819-
init(payload),
835+
resolvedChild,
820836
lanes,
821837
mergeDebugInfo(debugInfo, newChild._debugInfo),
822838
);
@@ -937,17 +953,24 @@ function createChildReconciler(
937953
debugInfo,
938954
);
939955
}
940-
case REACT_LAZY_TYPE:
941-
const payload = newChild._payload;
942-
const init = newChild._init;
956+
case REACT_LAZY_TYPE: {
957+
let resolvedChild;
958+
if (__DEV__) {
959+
resolvedChild = callLazyInitInDEV(newChild);
960+
} else {
961+
const payload = newChild._payload;
962+
const init = newChild._init;
963+
resolvedChild = init(payload);
964+
}
943965
return updateFromMap(
944966
existingChildren,
945967
returnFiber,
946968
newIdx,
947-
init(payload),
969+
resolvedChild,
948970
lanes,
949971
mergeDebugInfo(debugInfo, newChild._debugInfo),
950972
);
973+
}
951974
}
952975

953976
if (
@@ -1047,11 +1070,18 @@ function createChildReconciler(
10471070
key,
10481071
);
10491072
break;
1050-
case REACT_LAZY_TYPE:
1051-
const payload = child._payload;
1052-
const init = (child._init: any);
1053-
warnOnInvalidKey(init(payload), knownKeys, returnFiber);
1073+
case REACT_LAZY_TYPE: {
1074+
let resolvedChild;
1075+
if (__DEV__) {
1076+
resolvedChild = callLazyInitInDEV((child: any));
1077+
} else {
1078+
const payload = child._payload;
1079+
const init = (child._init: any);
1080+
resolvedChild = init(payload);
1081+
}
1082+
warnOnInvalidKey(resolvedChild, knownKeys, returnFiber);
10541083
break;
1084+
}
10551085
default:
10561086
break;
10571087
}

0 commit comments

Comments
 (0)