Skip to content

Commit

Permalink
Move createElement/JSX Warnings into the Renderer (#29088)
Browse files Browse the repository at this point in the history
This is necessary to simplify the component stack handling to make way
for owner stacks. It also solves some hacks that we used to have but
don't quite make sense. It also solves the problem where things like key
warnings get silenced in RSC because they get deduped. It also surfaces
areas where we were missing key warnings to begin with.

Almost every type of warning is issued from the renderer. React Elements
are really not anything special themselves. They're just lazily invoked
functions and its really the renderer that determines there semantics.

We have three types of warnings that previously fired in
JSX/createElement:

- Fragment props validation.
- Type validation.
- Key warning.

It's nice to be able to do some validation in the JSX/createElement
because it has a more specific stack frame at the callsite. However,
that's the case for every type of component and validation. That's the
whole point of enableOwnerStacks. It's also not sufficient to do it in
JSX/createElement so we also have validation in the renderers too. So
this validation is really just an eager validation but also happens
again later.

The problem with these is that we don't really know what types are valid
until we get to the renderer. Additionally, by placing it in the
isomorphic code it becomes harder to do deduping of warnings in a way
that makes sense for that renderer. It also means we can't reuse logic
for managing stacks etc.

Fragment props validation really should just be part of the renderer
like any other component type. This also matters once we add Fragment
refs and other fragment features. So I moved this into Fiber. However,
since some Fragments don't have Fibers, I do the validation in
ChildFiber instead of beginWork where it would normally happen.

For `type` validation we already do validation when rendering. By
leaving it to the renderer we don't have to hard code an extra list.
This list also varies by context. E.g. class components aren't allowed
in RSC but client references are but we don't have an isomorphic way to
identify client references because they're defined by the host config so
the current logic is flawed anyway. I kept the early validation for now
without the `enableOwnerStacks` since it does provide a nicer stack
frame but with that flag on it'll be handled with nice stacks anyway. I
normalized some of the errors to ensure tests pass.

For `key` validation it's the same principle. The mechanism for the
heuristic is still the same - if it passes statically through a parent
JSX/createElement call then it's considered validated. We already did
print the error later from the renderer so this also disables the early
log in the `enableOwnerStacks` flag.

I also added logging to Fizz so that key warnings can print in SSR logs.

Flight is a bit more complex. For elements that end up on the client we
just pass the `validated` flag along to the client and let the client
renderer print the error once rendered. For server components we log the
error from Flight with the server component as the owner on the stack
which will allow us to print the right stack for context. The factoring
of this is a little tricky because we only want to warn if it's in an
array parent but we want to log the error later to get the right debug
info.

Fiber/Fizz has a similar factoring problem that causes us to create a
fake Fiber for the owner which means the logs won't be associated with
the right place in DevTools.

DiffTrain build for commit 84239da.
  • Loading branch information
sebmarkbage committed May 23, 2024
1 parent bde36f5 commit a817127
Show file tree
Hide file tree
Showing 16 changed files with 529 additions and 333 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @noflow
* @nolint
* @preventMunge
* @generated SignedSource<<cc5ff8fe75f7cb7812ce459378d61959>>
* @generated SignedSource<<a12e42d980c252cd9449b3f2a99f483a>>
*/

'use strict';
Expand Down Expand Up @@ -44,7 +44,9 @@ var REACT_MEMO_TYPE = Symbol.for('react.memo');
var REACT_LAZY_TYPE = Symbol.for('react.lazy');
var REACT_OFFSCREEN_TYPE = Symbol.for('react.offscreen');

var REACT_CLIENT_REFERENCE = Symbol.for('react.client.reference');
var REACT_CLIENT_REFERENCE = Symbol.for('react.client.reference'); // This function is deprecated. Don't use. Only the renderer knows what a valid type is.
// TODO: Delete this when enableOwnerStacks ships.

function isValidElementType(type) {
if (typeof type === 'string' || typeof type === 'function') {
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @noflow
* @nolint
* @preventMunge
* @generated SignedSource<<98724556db7e01bfefca7439ac7adfc3>>
* @generated SignedSource<<3f7c609322e65db1e2ce38a0f7c72577>>
*/

'use strict';
Expand Down Expand Up @@ -55,11 +55,14 @@ function printWarning(level, format, args) {
// update consoleWithStackDev.www.js as well.
{
var isErrorLogger = format === '%s\n\n%s\n' || format === '%o\n\n%s\n\n%s\n';
var stack = ReactSharedInternals.getStackAddendum();

if (stack !== '') {
format += '%s';
args = args.concat([stack]);
if (ReactSharedInternals.getCurrentStack) {
var stack = ReactSharedInternals.getCurrentStack();

if (stack !== '') {
format += '%s';
args = args.concat([stack]);
}
}

if (isErrorLogger) {
Expand Down Expand Up @@ -5631,7 +5634,7 @@ var warnForMissingKey = function (child, returnFiber) {};
return;
}

if (!child._store || child._store.validated || child.key != null) {
if (!child._store || (child._store.validated || child.key != null) && child._store.validated !== 2) {
return;
}

Expand All @@ -5640,17 +5643,96 @@ var warnForMissingKey = function (child, returnFiber) {};
} // $FlowFixMe[cannot-write] unable to narrow type from mixed to writable object


child._store.validated = true;
var componentName = getComponentNameFromFiber(returnFiber) || 'Component';
child._store.validated = 1;
var componentName = getComponentNameFromFiber(returnFiber);
var componentKey = componentName || 'null';

if (ownerHasKeyUseWarning[componentName]) {
if (ownerHasKeyUseWarning[componentKey]) {
return;
}

ownerHasKeyUseWarning[componentName] = true;
ownerHasKeyUseWarning[componentKey] = true;
var childOwner = child._owner;
var parentOwner = returnFiber._debugOwner;
var currentComponentErrorInfo = '';

if (parentOwner && typeof parentOwner.tag === 'number') {
var name = getComponentNameFromFiber(parentOwner);

if (name) {
currentComponentErrorInfo = '\n\nCheck the render method of `' + name + '`.';
}
}

if (!currentComponentErrorInfo) {
if (componentName) {
currentComponentErrorInfo = "\n\nCheck the top-level render call using <" + componentName + ">.";
}
} // Usually the current owner is the offender, but if it accepts children as a
// property, it may be the creator of the child that's responsible for
// assigning it a key.


var childOwnerAppendix = '';

if (childOwner != null && parentOwner !== childOwner) {
var ownerName = null;

if (typeof childOwner.tag === 'number') {
ownerName = getComponentNameFromFiber(childOwner);
} else if (typeof childOwner.name === 'string') {
ownerName = childOwner.name;
}

if (ownerName) {
// Give the component that originally created this child.
childOwnerAppendix = " It was passed a child from " + ownerName + ".";
}
} // We create a fake Fiber for the child to log the stack trace from.
// TODO: Refactor the warnForMissingKey calls to happen after fiber creation
// so that we can get access to the fiber that will eventually be created.
// That way the log can show up associated with the right instance in DevTools.


var fiber = createFiberFromElement(child, returnFiber.mode, 0);
fiber.return = returnFiber;
var prevDebugFiber = getCurrentFiber();
setCurrentFiber(fiber);

error('Each child in a list should have a unique "key" prop.' + '%s%s See https://react.dev/link/warning-keys for more information.', currentComponentErrorInfo, childOwnerAppendix);

error('Each child in a list should have a unique ' + '"key" prop. See https://react.dev/link/warning-keys for ' + 'more information.');
setCurrentFiber(prevDebugFiber);
};
} // Given a fragment, validate that it can only be provided with fragment props
// We do this here instead of BeginWork because the Fragment fiber doesn't have
// the whole props object, only the children and is shared with arrays.


function validateFragmentProps(element, fiber, returnFiber) {
{
var keys = Object.keys(element.props);

for (var i = 0; i < keys.length; i++) {
var key = keys[i];

if (key !== 'children' && key !== 'key') {
if (fiber === null) {
// For unkeyed root fragments there's no Fiber. We create a fake one just for
// error stack handling.
fiber = createFiberFromElement(element, returnFiber.mode, 0);
fiber.return = returnFiber;
}

var prevDebugFiber = getCurrentFiber();
setCurrentFiber(fiber);

error('Invalid prop `%s` supplied to `React.Fragment`. ' + 'React.Fragment can only have `key` and `children` props.', key);

setCurrentFiber(prevDebugFiber);
break;
}
}
}
}

function unwrapThenable(thenable) {
Expand Down Expand Up @@ -5871,7 +5953,9 @@ function createChildReconciler(shouldTrackSideEffects) {
var elementType = element.type;

if (elementType === REACT_FRAGMENT_TYPE) {
return updateFragment(returnFiber, current, element.props.children, lanes, element.key, debugInfo);
var updated = updateFragment(returnFiber, current, element.props.children, lanes, element.key, debugInfo);
validateFragmentProps(element, updated, returnFiber);
return updated;
}

if (current !== null) {
Expand Down Expand Up @@ -6615,6 +6699,7 @@ function createChildReconciler(shouldTrackSideEffects) {
existing._debugInfo = debugInfo;
}

validateFragmentProps(element, existing, returnFiber);
return existing;
}
} else {
Expand Down Expand Up @@ -6658,6 +6743,7 @@ function createChildReconciler(shouldTrackSideEffects) {
created._debugInfo = debugInfo;
}

validateFragmentProps(element, created, returnFiber);
return created;
} else {
var _created4 = createFiberFromElement(element, returnFiber.mode, lanes);
Expand Down Expand Up @@ -6720,6 +6806,7 @@ function createChildReconciler(shouldTrackSideEffects) {
var isUnkeyedTopLevelFragment = typeof newChild === 'object' && newChild !== null && newChild.type === REACT_FRAGMENT_TYPE && newChild.key === null;

if (isUnkeyedTopLevelFragment) {
validateFragmentProps(newChild, null, returnFiber);
newChild = newChild.props.children;
} // Handle object types

Expand Down Expand Up @@ -23122,10 +23209,22 @@ key, pendingProps, owner, mode, lanes) {
}

var info = '';
var typeString;

{
if (type === undefined || typeof type === 'object' && type !== null && Object.keys(type).length === 0) {
info += ' You likely forgot to export your component from the file ' + "it's defined in, or you might have mixed up default and " + 'named imports.';
info += ' You likely forgot to export your component from the file ' + "it's defined in, or you might have mixed up default and named imports.";
}

if (type === null) {
typeString = 'null';
} else if (isArray(type)) {
typeString = 'array';
} else if (type !== undefined && type.$$typeof === REACT_ELEMENT_TYPE) {
typeString = "<" + (getComponentNameFromType(type.type) || 'Unknown') + " />";
info = ' Did you accidentally export a JSX literal instead of a component?';
} else {
typeString = typeof type;
}

var ownerName = owner ? getComponentNameFromOwner(owner) : null;
Expand All @@ -23135,7 +23234,7 @@ key, pendingProps, owner, mode, lanes) {
}
}

throw new Error('Element type is invalid: expected a string (for built-in ' + 'components) or a class/function (for composite components) ' + ("but got: " + (type == null ? type : typeof type) + "." + info));
throw new Error('Element type is invalid: expected a string (for built-in ' + 'components) or a class/function (for composite components) ' + ("but got: " + typeString + "." + info));
}
}
}
Expand Down Expand Up @@ -23344,7 +23443,7 @@ identifierPrefix, onUncaughtError, onCaughtError, onRecoverableError, transition
return root;
}

var ReactVersion = '19.0.0-rc-47384982';
var ReactVersion = '19.0.0-rc-c6122275';

/*
* The `'' + value` pattern (used in perf-sensitive code) throws for Symbol
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @noflow
* @nolint
* @preventMunge
* @generated SignedSource<<6540b866a6ff4178bae5e8496c9c2dc6>>
* @generated SignedSource<<b3af25d61554f4397e525034ff4d9bc6>>
*/

"use strict";
Expand Down Expand Up @@ -8774,7 +8774,7 @@ function createFiberFromTypeAndProps(
}
throw Error(
"Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: " +
((null == type ? type : typeof type) + ".")
((null === type ? "null" : typeof type) + ".")
);
}
key = createFiber(fiberTag, pendingProps, key, mode);
Expand Down Expand Up @@ -9300,7 +9300,7 @@ var devToolsConfig$jscomp$inline_1042 = {
throw Error("TestRenderer does not support findFiberByHostInstance()");
},
bundleType: 0,
version: "19.0.0-rc-59df980e",
version: "19.0.0-rc-382da410",
rendererPackageName: "react-test-renderer"
};
var internals$jscomp$inline_1229 = {
Expand Down Expand Up @@ -9331,7 +9331,7 @@ var internals$jscomp$inline_1229 = {
scheduleRoot: null,
setRefreshHandler: null,
getCurrentFiber: null,
reconcilerVersion: "19.0.0-rc-59df980e"
reconcilerVersion: "19.0.0-rc-382da410"
};
if ("undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__) {
var hook$jscomp$inline_1230 = __REACT_DEVTOOLS_GLOBAL_HOOK__;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @noflow
* @nolint
* @preventMunge
* @generated SignedSource<<9640a2643c047b950331599cf3b70c9c>>
* @generated SignedSource<<ebfa53ae43ecaabc34a7bb3185aba21e>>
*/

"use strict";
Expand Down Expand Up @@ -9412,7 +9412,7 @@ function createFiberFromTypeAndProps(
}
throw Error(
"Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: " +
((null == type ? type : typeof type) + ".")
((null === type ? "null" : typeof type) + ".")
);
}
key = createFiber(fiberTag, pendingProps, key, mode);
Expand Down Expand Up @@ -9943,7 +9943,7 @@ var devToolsConfig$jscomp$inline_1105 = {
throw Error("TestRenderer does not support findFiberByHostInstance()");
},
bundleType: 0,
version: "19.0.0-rc-98cdfed4",
version: "19.0.0-rc-7fcc15f0",
rendererPackageName: "react-test-renderer"
};
(function (internals) {
Expand Down Expand Up @@ -9987,7 +9987,7 @@ var devToolsConfig$jscomp$inline_1105 = {
scheduleRoot: null,
setRefreshHandler: null,
getCurrentFiber: null,
reconcilerVersion: "19.0.0-rc-98cdfed4"
reconcilerVersion: "19.0.0-rc-7fcc15f0"
});
exports._Scheduler = Scheduler;
exports.act = act;
Expand Down
Loading

0 comments on commit a817127

Please sign in to comment.