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.

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

[Fiber] Use Owner/JSX Stack When Appending Stacks to Console #29206

Merged
merged 7 commits into from
May 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,7 @@ module.exports = {
$ReadOnlyArray: 'readonly',
$ArrayBufferView: 'readonly',
$Shape: 'readonly',
ConsoleTask: 'readonly', // TOOD: Figure out what the official name of this will be.
ReturnType: 'readonly',
AnimationFrameID: 'readonly',
// For Flow type annotation. Only `BigInt` is valid at runtime.
Expand Down
16 changes: 8 additions & 8 deletions packages/react-client/src/__tests__/ReactFlight-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1123,10 +1123,11 @@ describe('ReactFlight', () => {
}

function App() {
return (
<Indirection>
<ClientComponent />
</Indirection>
// We use the ReactServer runtime here to get the Server owner.
return ReactServer.createElement(
Indirection,
null,
ReactServer.createElement(ClientComponent),
);
}

Expand All @@ -1143,11 +1144,10 @@ describe('ReactFlight', () => {
'\n' +
'Check the render method of `Component`. See https://react.dev/link/warning-keys for more information.\n' +
' in span (at **)\n' +
// TODO: Because this validates after the div has been mounted, it is part of
// the parent stack but since owner stacks will switch to owners this goes away again.
(gate(flags => flags.enableOwnerStacks) ? ' in div (at **)\n' : '') +
' in Component (at **)\n' +
' in Indirection (at **)\n' +
(gate(flags => flags.enableOwnerStacks)
? ''
: ' in Indirection (at **)\n') +
' in App (at **)',
);
});
Expand Down
50 changes: 38 additions & 12 deletions packages/react-dom-bindings/src/client/validateDOMNesting.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
* @flow
*/

import {getCurrentParentStackInDev} from 'react-reconciler/src/ReactCurrentFiber';

type Info = {tag: string};
export type AncestorInfoDev = {
current: ?Info,
Expand Down Expand Up @@ -476,19 +478,31 @@ function validateDOMNesting(
' Add a <tbody>, <thead> or <tfoot> to your code to match the DOM tree generated by ' +
'the browser.';
}
console.error(
'In HTML, %s cannot be a child of <%s>.%s\n' +
'This will cause a hydration error.',
// Don't transform into consoleWithStackDev here because we add a manual stack.
// We use the parent stack here instead of the owner stack because the parent
// stack has more useful context for nesting.
// TODO: Format this as a linkified "diff view" with props instead of
// a stack trace since the stack trace format is now for owner stacks.
console['error'](
'Warning: In HTML, %s cannot be a child of <%s>.%s\n' +
'This will cause a hydration error.%s',
tagDisplayName,
ancestorTag,
info,
getCurrentParentStackInDev(),
);
} else {
console.error(
'In HTML, %s cannot be a descendant of <%s>.\n' +
'This will cause a hydration error.',
// Don't transform into consoleWithStackDev here because we add a manual stack.
// We use the parent stack here instead of the owner stack because the parent
// stack has more useful context for nesting.
// TODO: Format this as a linkified "diff view" with props instead of
// a stack trace since the stack trace format is now for owner stacks.
console['error'](
'Warning: In HTML, %s cannot be a descendant of <%s>.\n' +
'This will cause a hydration error.%s',
tagDisplayName,
ancestorTag,
getCurrentParentStackInDev(),
);
}
return false;
Expand All @@ -510,18 +524,30 @@ function validateTextNesting(childText: string, parentTag: string): boolean {
didWarn[warnKey] = true;

if (/\S/.test(childText)) {
console.error(
'In HTML, text nodes cannot be a child of <%s>.\n' +
'This will cause a hydration error.',
// Don't transform into consoleWithStackDev here because we add a manual stack.
// We use the parent stack here instead of the owner stack because the parent
// stack has more useful context for nesting.
// TODO: Format this as a linkified "diff view" with props instead of
// a stack trace since the stack trace format is now for owner stacks.
console['error'](
'Warning: In HTML, text nodes cannot be a child of <%s>.\n' +
'This will cause a hydration error.%s',
parentTag,
getCurrentParentStackInDev(),
);
} else {
console.error(
'In HTML, whitespace text nodes cannot be a child of <%s>. ' +
// Don't transform into consoleWithStackDev here because we add a manual stack.
// We use the parent stack here instead of the owner stack because the parent
// stack has more useful context for nesting.
// TODO: Format this as a linkified "diff view" with props instead of
// a stack trace since the stack trace format is now for owner stacks.
console['error'](
'Warning: In HTML, whitespace text nodes cannot be a child of <%s>. ' +
"Make sure you don't have any extra whitespace between tags on " +
'each line of your source code.\n' +
'This will cause a hydration error.',
'This will cause a hydration error.%s',
parentTag,
getCurrentParentStackInDev(),
);
}
return false;
Expand Down
8 changes: 6 additions & 2 deletions packages/react-dom/src/__tests__/ReactChildReconciler-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,9 @@ describe('ReactChildReconciler', () => {
'could change in a future version.\n' +
' in div (at **)\n' +
' in Component (at **)\n' +
' in Parent (at **)\n' +
(gate(flags => flags.enableOwnerStacks)
? ''
: ' in Parent (at **)\n') +
' in GrandParent (at **)',
);
});
Expand Down Expand Up @@ -189,7 +191,9 @@ describe('ReactChildReconciler', () => {
'could change in a future version.\n' +
' in div (at **)\n' +
' in Component (at **)\n' +
' in Parent (at **)\n' +
(gate(flags => flags.enableOwnerStacks)
? ''
: ' in Parent (at **)\n') +
' in GrandParent (at **)',
);
});
Expand Down
8 changes: 6 additions & 2 deletions packages/react-dom/src/__tests__/ReactComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,9 @@ describe('ReactComponent', () => {
'Or maybe you meant to call this function rather than return it.\n' +
' <span>{Foo}</span>\n' +
' in span (at **)\n' +
' in div (at **)\n' +
(gate(flags => flags.enableOwnerStacks)
? ''
: ' in div (at **)\n') +
' in Foo (at **)',
);
});
Expand Down Expand Up @@ -820,7 +822,9 @@ describe('ReactComponent', () => {
'Or maybe you meant to call this function rather than return it.\n' +
' <span>{Foo}</span>\n' +
' in span (at **)\n' +
' in div (at **)\n' +
(gate(flags => flags.enableOwnerStacks)
? ''
: ' in div (at **)\n') +
' in Foo (at **)',
]);
await act(() => {
Expand Down
4 changes: 2 additions & 2 deletions packages/react-dom/src/__tests__/ReactDOM-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,7 @@ describe('ReactDOM', () => {
// ReactDOM(App > div > span)
'Invalid ARIA attribute `ariaTypo`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in span (at **)\n' +
' in div (at **)\n' +
(gate(flags => flags.enableOwnerStacks) ? '' : ' in div (at **)\n') +
' in App (at **)',
// ReactDOM(App > div > ServerEntry) >>> ReactDOMServer(Child) >>> ReactDOMServer(App2) >>> ReactDOMServer(blink)
'Invalid ARIA attribute `ariaTypo2`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
Expand All @@ -569,7 +569,7 @@ describe('ReactDOM', () => {
// ReactDOM(App > div > font)
'Invalid ARIA attribute `ariaTypo5`. ARIA attributes follow the pattern aria-* and must be lowercase.\n' +
' in font (at **)\n' +
' in div (at **)\n' +
(gate(flags => flags.enableOwnerStacks) ? '' : ' in div (at **)\n') +
' in App (at **)',
]);
});
Expand Down
8 changes: 6 additions & 2 deletions packages/react-dom/src/__tests__/ReactMultiChild-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,9 @@ describe('ReactMultiChild', () => {
'could change in a future version.\n' +
' in div (at **)\n' +
' in WrapperComponent (at **)\n' +
' in div (at **)\n' +
(gate(flags => flags.enableOwnerStacks)
? ''
: ' in div (at **)\n') +
' in Parent (at **)',
);
});
Expand Down Expand Up @@ -292,7 +294,9 @@ describe('ReactMultiChild', () => {
'could change in a future version.\n' +
' in div (at **)\n' +
' in WrapperComponent (at **)\n' +
' in div (at **)\n' +
(gate(flags => flags.enableOwnerStacks)
? ''
: ' in div (at **)\n') +
' in Parent (at **)',
);
});
Expand Down
13 changes: 11 additions & 2 deletions packages/react-dom/src/__tests__/ReactUpdates-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1848,7 +1848,7 @@ describe('ReactUpdates', () => {
it('warns about a deferred infinite update loop with useEffect', async () => {
function NonTerminating() {
const [step, setStep] = React.useState(0);
React.useEffect(() => {
React.useEffect(function myEffect() {
setStep(x => x + 1);
});
return step;
Expand All @@ -1860,10 +1860,12 @@ describe('ReactUpdates', () => {

let error = null;
let stack = null;
let nativeStack = null;
const originalConsoleError = console.error;
console.error = (e, s) => {
error = e;
stack = s;
nativeStack = new Error().stack;
Scheduler.log('stop');
};
try {
Expand All @@ -1876,7 +1878,14 @@ describe('ReactUpdates', () => {
}

expect(error).toContain('Maximum update depth exceeded');
expect(stack).toContain('at NonTerminating');
// The currently executing effect should be on the native stack
expect(nativeStack).toContain('at myEffect');
if (!gate(flags => flags.enableOwnerStacks)) {
// The currently running component's name is not in the owner
// stack because it's just its JSX callsite.
expect(stack).toContain('at NonTerminating');
}
expect(stack).toContain('at App');
});

it('can have nested updates if they do not cross the limit', async () => {
Expand Down
58 changes: 44 additions & 14 deletions packages/react-reconciler/src/ReactChildFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ import {getIsHydrating} from './ReactFiberHydrationContext';
import {pushTreeFork} from './ReactFiberTreeContext';
import {createThenableState, trackUsedThenable} from './ReactFiberThenable';
import {readContextDuringReconciliation} from './ReactFiberNewContext';
import {callLazyInitInDEV} from './ReactFiberCallUserSpace';

import {
getCurrentFiber as getCurrentDebugFiberInDEV,
Expand Down Expand Up @@ -362,6 +363,9 @@ function warnOnSymbolType(returnFiber: Fiber, invalidChild: symbol) {
}

function resolveLazy(lazyType: any) {
if (__DEV__) {
return callLazyInitInDEV(lazyType);
}
const payload = lazyType._payload;
const init = lazyType._init;
return init(payload);
Expand Down Expand Up @@ -683,11 +687,17 @@ function createChildReconciler(
return created;
}
case REACT_LAZY_TYPE: {
const payload = newChild._payload;
const init = newChild._init;
let resolvedChild;
if (__DEV__) {
resolvedChild = callLazyInitInDEV(newChild);
} else {
const payload = newChild._payload;
const init = newChild._init;
resolvedChild = init(payload);
}
return createChild(
returnFiber,
init(payload),
resolvedChild,
lanes,
mergeDebugInfo(debugInfo, newChild._debugInfo), // call merge after init
);
Expand Down Expand Up @@ -811,12 +821,18 @@ function createChildReconciler(
}
}
case REACT_LAZY_TYPE: {
const payload = newChild._payload;
const init = newChild._init;
let resolvedChild;
if (__DEV__) {
resolvedChild = callLazyInitInDEV(newChild);
} else {
const payload = newChild._payload;
const init = newChild._init;
resolvedChild = init(payload);
}
return updateSlot(
returnFiber,
oldFiber,
init(payload),
resolvedChild,
lanes,
mergeDebugInfo(debugInfo, newChild._debugInfo),
);
Expand Down Expand Up @@ -937,17 +953,24 @@ function createChildReconciler(
debugInfo,
);
}
case REACT_LAZY_TYPE:
const payload = newChild._payload;
const init = newChild._init;
case REACT_LAZY_TYPE: {
let resolvedChild;
if (__DEV__) {
resolvedChild = callLazyInitInDEV(newChild);
} else {
const payload = newChild._payload;
const init = newChild._init;
resolvedChild = init(payload);
}
return updateFromMap(
existingChildren,
returnFiber,
newIdx,
init(payload),
resolvedChild,
lanes,
mergeDebugInfo(debugInfo, newChild._debugInfo),
);
}
}

if (
Expand Down Expand Up @@ -1047,11 +1070,18 @@ function createChildReconciler(
key,
);
break;
case REACT_LAZY_TYPE:
const payload = child._payload;
const init = (child._init: any);
warnOnInvalidKey(init(payload), knownKeys, returnFiber);
case REACT_LAZY_TYPE: {
let resolvedChild;
if (__DEV__) {
resolvedChild = callLazyInitInDEV((child: any));
} else {
const payload = child._payload;
const init = (child._init: any);
resolvedChild = init(payload);
}
warnOnInvalidKey(resolvedChild, knownKeys, returnFiber);
break;
}
default:
break;
}
Expand Down
Loading