Skip to content

Commit

Permalink
Improve debugging DX when string coercions fail
Browse files Browse the repository at this point in the history
If a coercion throws, the check function will now throw after it calls
console.error(). It's the same exception that will be thrown after
the check function returns, but by throwing it inside of the check
function, it will be easier for callers to understand what happened
and how to fix it.

I added a bunch of comments above where it throws, so that callers who
follow links from a call stack will see troubleshooting info.

This commit also fixes a mistake where one block of DEV-only code
wasn't included in an `if (__DEV__) {}` block.
  • Loading branch information
justingrant committed Sep 20, 2021
1 parent f7a0e55 commit 29bc544
Showing 1 changed file with 43 additions and 18 deletions.
61 changes: 43 additions & 18 deletions packages/shared/CheckStringCoercion.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,33 +34,52 @@ function typeName(value: mixed): string {

// $FlowFixMe only called in DEV, so void return is not possible.
function willCoercionThrow(value: mixed): boolean {
if (
value !== null &&
typeof value === 'object' &&
value.$$typeof === REACT_OPAQUE_ID_TYPE
) {
// OpaqueID type is expected to throw, so React will handle it. Not sure if
// it's expected that string coercion will throw, but we'll assume it's OK.
// See https://github.com/facebook/react/issues/20127.
return false;
}
if (__DEV__) {
if (
value !== null &&
typeof value === 'object' &&
value.$$typeof === REACT_OPAQUE_ID_TYPE
) {
// OpaqueID type is expected to throw, so React will handle it. Not sure if
// it's expected that string coercion will throw, but we'll assume it's OK.
// See https://github.com/facebook/react/issues/20127.
return;
}
try {
// eslint-disable-next-line react-internal/safe-string-coercion
void ('' + (value: any));
testStringCoercion(value);
return false;
} catch (e) {
return true;
}
}
}

// TODO: what is React's "dev-only invariant"? I don't only want to show a
// console error because the next line after this returns will crash, so the
// whole point of these functions is to crash here, not after it returns. The
// invariant implementation does neat things with exception handling in DEV. But
// the lint message from invariant sounds like it will have a prod footprint,
// but this is dev-only code. What to do?
function testStringCoercion(value: mixed) {
// If you ended up here by following an exception call stack, here's what's
// happened: you supplied an object or symbol value to React (as a prop, key,
// DOM attribute, CSS property, string ref, etc.) and when React tried to
// coerce it to a string using `'' + value`, an exception was thrown.
//
// The most common types that will cause this exception are `Symbol` instances
// and Temporal objects like `Temporal.Instant`. But any object that has a
// `valueOf` or `[Symbol.toPrimitive]` method that throws will also cause this
// exception. (Library authors do this to prevent users from using built-in
// numeric operators like `+` or comparison operators like `>=` because custom
// methods are needed to perform accurate arithmetic or comparison.)
//
// To fix the problem, coerce this object or symbol value to a string before
// passing it to React. The most reliable way is usually `String(value)`.
//
// To find which value is throwing, check the browser or debugger console.
// Before this exception was thrown, there should be `console.error` output
// that shows the type (Symbol, Temporal.PlainDate, etc.) that caused the
// problem and how that type was used: key, atrribute, input value prop, etc.
// In most cases, this console output also shows the component and its
// ancestor components where the exception happened.
//
// eslint-disable-next-line react-internal/safe-string-coercion
return '' + (value: any);
}

export function checkAttributeStringCoercion(
value: mixed,
Expand All @@ -74,6 +93,7 @@ export function checkAttributeStringCoercion(
attributeName,
typeName(value),
);
return testStringCoercion(value); // throw (to help callers find troubleshooting comments)
}
}
}
Expand All @@ -86,6 +106,7 @@ export function checkKeyStringCoercion(value: mixed) {
' This value must be coerced to a string before before using it here.',
typeName(value),
);
return testStringCoercion(value); // throw (to help callers find troubleshooting comments)
}
}
}
Expand All @@ -99,6 +120,7 @@ export function checkPropStringCoercion(value: mixed, propName: string) {
propName,
typeName(value),
);
return testStringCoercion(value); // throw (to help callers find troubleshooting comments)
}
}
}
Expand All @@ -112,6 +134,7 @@ export function checkCSSPropertyStringCoercion(value: mixed, propName: string) {
propName,
typeName(value),
);
return testStringCoercion(value); // throw (to help callers find troubleshooting comments)
}
}
}
Expand All @@ -124,6 +147,7 @@ export function checkHtmlStringCoercion(value: mixed) {
' This value must be coerced to a string before before using it here.',
typeName(value),
);
return testStringCoercion(value); // throw (to help callers find troubleshooting comments)
}
}
}
Expand All @@ -137,6 +161,7 @@ export function checkFormFieldValueStringCoercion(value: mixed) {
' This value must be coerced to a string before before using it here.',
typeName(value),
);
return testStringCoercion(value); // throw (to help callers find troubleshooting comments)
}
}
}

0 comments on commit 29bc544

Please sign in to comment.