Skip to content

Commit

Permalink
Remove Warning: prefix and toString on console Arguments (#29839)
Browse files Browse the repository at this point in the history
Basically make `console.error` and `console.warn` behave like normal -
when a component stack isn't appended. I need this because I need to be
able to print rich logs with the component stack option and to be able
to disable instrumentation completely in `console.createTask`
environments that don't need it.

Currently we can't print logs with richer objects because they're
toString:ed first. In practice, pretty much all arguments we log are
already toString:ed so it's not necessary anyway. Some might be like a
number. So it would only be a problem if some environment can't handle
proper consoles but then it's up to that environment to toString it
before logging.

The `Warning: ` prefix is historic and is both noisy and confusing. It's
mostly unnecessary since the UI surrounding `console.error` and
`console.warn` tend to have visual treatment around it anyway. However,
it's actively misleading when `console.error` gets prefixed with a
Warning that we consider an error level. There's an argument to be made
that some of our `console.error` don't make the bar for an error but
then the argument is to downgrade each of those to `console.warn` - not
to brand all our actual error logging with `Warning: `.

Apparently something needs to change in React Native before landing this
because it depends on the prefix somehow which probably doesn't make
sense already.
  • Loading branch information
sebmarkbage authored Jun 10, 2024
1 parent d172bda commit 2774208
Show file tree
Hide file tree
Showing 77 changed files with 397 additions and 418 deletions.
3 changes: 3 additions & 0 deletions packages/internal-test-utils/shouldIgnoreConsoleError.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ module.exports = function shouldIgnoreConsoleError(format, args) {
args[0] != null &&
((typeof args[0] === 'object' &&
typeof args[0].message === 'string' &&
// This specific log has the same signature as error logging.
// The trick is to get rid of this whole file.
!format.includes('Failed to serialize an action') &&
typeof args[0].stack === 'string') ||
(typeof args[0] === 'string' &&
args[0].indexOf('An error occurred in ') === 0))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ describe('ReactCache', () => {
await waitForAll(['App', 'Loading...']);
}).toErrorDev([
'Invalid key type. Expected a string, number, symbol, or ' +
'boolean, but instead received: Hi,100\n\n' +
"boolean, but instead received: [ 'Hi', 100 ]\n\n" +
'To use non-primitive values as keys, you must pass a hash ' +
'function as the second argument to createResource().',
]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2556,7 +2556,7 @@ describe('InspectedElement', () => {
};

await withErrorsOrWarningsIgnored(
['Warning: Each child in a list should have a unique "key" prop.'],
['Each child in a list should have a unique "key" prop.'],
async () => {
await utils.actAsync(() =>
render(<Example repeatWarningCount={1} />),
Expand Down
21 changes: 10 additions & 11 deletions packages/react-devtools-shared/src/__tests__/setupTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,23 +154,22 @@ beforeEach(() => {

const originalConsoleError = console.error;
console.error = (...args) => {
const firstArg = args[0];
if (
firstArg === 'Warning: React instrumentation encountered an error: %s'
) {
let firstArg = args[0];
if (typeof firstArg === 'string' && firstArg.startsWith('Warning: ')) {
// Older React versions might use the Warning: prefix. I'm not sure
// if they use this code path.
firstArg = firstArg.slice(9);
}
if (firstArg === 'React instrumentation encountered an error: %s') {
// Rethrow errors from React.
throw args[1];
} else if (
typeof firstArg === 'string' &&
(firstArg.startsWith(
"Warning: It looks like you're using the wrong act()",
) ||
(firstArg.startsWith("It looks like you're using the wrong act()") ||
firstArg.startsWith(
'Warning: The current testing environment is not configured to support act',
'The current testing environment is not configured to support act',
) ||
firstArg.startsWith(
'Warning: You seem to have overlapping act() calls',
))
firstArg.startsWith('You seem to have overlapping act() calls'))
) {
// DevTools intentionally wraps updates with acts from both DOM and test-renderer,
// since test updates are expected to impact both renderers.
Expand Down
4 changes: 2 additions & 2 deletions packages/react-devtools-shared/src/__tests__/store-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1927,7 +1927,7 @@ describe('Store', () => {
}

withErrorsOrWarningsIgnored(
['Warning: Each child in a list should have a unique "key" prop'],
['Each child in a list should have a unique "key" prop'],
() => {
act(() => render(<Example />));
},
Expand All @@ -1952,7 +1952,7 @@ describe('Store', () => {
}

withErrorsOrWarningsIgnored(
['Warning: Each child in a list should have a unique "key" prop'],
['Each child in a list should have a unique "key" prop'],
() => {
act(() => render(<Example />));
},
Expand Down
12 changes: 11 additions & 1 deletion packages/react-devtools-shell/src/app/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,18 @@ ignoreErrors([
'Warning: %s is deprecated in StrictMode.', // findDOMNode
'Warning: ReactDOM.render was removed in React 19',
'Warning: react-test-renderer is deprecated',
// Ignore prefixed and not prefixed since I don't know which
// React versions are being tested by this code.
'Legacy context API',
'Unsafe lifecycle methods',
'%s is deprecated in StrictMode.', // findDOMNode
'ReactDOM.render was removed in React 19',
'react-test-renderer is deprecated',
]);
ignoreWarnings([
'Warning: componentWillReceiveProps has been renamed',
'componentWillReceiveProps has been renamed',
]);
ignoreWarnings(['Warning: componentWillReceiveProps has been renamed']);
ignoreLogs([]);

const unmountFunctions: Array<() => void | boolean> = [];
Expand Down
8 changes: 4 additions & 4 deletions packages/react-dom-bindings/src/client/validateDOMNesting.js
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ function validateDOMNesting(
// 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' +
'In HTML, %s cannot be a child of <%s>.%s\n' +
'This will cause a hydration error.%s',
tagDisplayName,
ancestorTag,
Expand All @@ -498,7 +498,7 @@ function validateDOMNesting(
// 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' +
'In HTML, %s cannot be a descendant of <%s>.\n' +
'This will cause a hydration error.%s',
tagDisplayName,
ancestorTag,
Expand Down Expand Up @@ -530,7 +530,7 @@ function validateTextNesting(childText: string, parentTag: string): boolean {
// 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' +
'In HTML, text nodes cannot be a child of <%s>.\n' +
'This will cause a hydration error.%s',
parentTag,
getCurrentParentStackInDev(),
Expand All @@ -542,7 +542,7 @@ function validateTextNesting(childText: string, parentTag: string): boolean {
// 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>. ' +
'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.%s',
Expand Down
18 changes: 9 additions & 9 deletions packages/react-dom/src/__tests__/CSSPropertyOperations-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ describe('CSSPropertyOperations', () => {
root.render(<Comp />);
});
}).toErrorDev(
'Warning: Unsupported style property background-color. Did you mean backgroundColor?' +
'Unsupported style property background-color. Did you mean backgroundColor?' +
'\n in div (at **)' +
'\n in Comp (at **)',
);
Expand Down Expand Up @@ -137,10 +137,10 @@ describe('CSSPropertyOperations', () => {
root.render(<Comp style={styles} />);
});
}).toErrorDev([
'Warning: Unsupported style property -ms-transform. Did you mean msTransform?' +
'Unsupported style property -ms-transform. Did you mean msTransform?' +
'\n in div (at **)' +
'\n in Comp (at **)',
'Warning: Unsupported style property -webkit-transform. Did you mean WebkitTransform?' +
'Unsupported style property -webkit-transform. Did you mean WebkitTransform?' +
'\n in div (at **)' +
'\n in Comp (at **)',
]);
Expand Down Expand Up @@ -171,11 +171,11 @@ describe('CSSPropertyOperations', () => {
});
}).toErrorDev([
// msTransform is correct already and shouldn't warn
'Warning: Unsupported vendor-prefixed style property oTransform. ' +
'Unsupported vendor-prefixed style property oTransform. ' +
'Did you mean OTransform?' +
'\n in div (at **)' +
'\n in Comp (at **)',
'Warning: Unsupported vendor-prefixed style property webkitTransform. ' +
'Unsupported vendor-prefixed style property webkitTransform. ' +
'Did you mean WebkitTransform?' +
'\n in div (at **)' +
'\n in Comp (at **)',
Expand Down Expand Up @@ -207,11 +207,11 @@ describe('CSSPropertyOperations', () => {
root.render(<Comp />);
});
}).toErrorDev([
"Warning: Style property values shouldn't contain a semicolon. " +
"Style property values shouldn't contain a semicolon. " +
'Try "backgroundColor: blue" instead.' +
'\n in div (at **)' +
'\n in Comp (at **)',
"Warning: Style property values shouldn't contain a semicolon. " +
"Style property values shouldn't contain a semicolon. " +
'Try "color: red" instead.' +
'\n in div (at **)' +
'\n in Comp (at **)',
Expand All @@ -234,7 +234,7 @@ describe('CSSPropertyOperations', () => {
root.render(<Comp />);
});
}).toErrorDev(
'Warning: `NaN` is an invalid value for the `fontSize` css style property.' +
'`NaN` is an invalid value for the `fontSize` css style property.' +
'\n in div (at **)' +
'\n in Comp (at **)',
);
Expand Down Expand Up @@ -270,7 +270,7 @@ describe('CSSPropertyOperations', () => {
root.render(<Comp />);
});
}).toErrorDev(
'Warning: `Infinity` is an invalid value for the `fontSize` css style property.' +
'`Infinity` is an invalid value for the `fontSize` css style property.' +
'\n in div (at **)' +
'\n in Comp (at **)',
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1333,7 +1333,7 @@ describe('DOMPropertyOperations', () => {
});

assertConsoleErrorDev([
'The `popoverTarget` prop expects the ID of an Element as a string. Received [object HTMLDivElement] instead.',
'The `popoverTarget` prop expects the ID of an Element as a string. Received HTMLDivElement {} instead.',
]);

// Dedupe warning
Expand Down
12 changes: 6 additions & 6 deletions packages/react-dom/src/__tests__/ReactComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ describe('ReactComponent', () => {
root.render(<Component />);
});
}).toErrorDev([
'Warning: Component "Component" contains the string ref "inner". ' +
'Component "Component" contains the string ref "inner". ' +
'Support for string refs will be removed in a future major release. ' +
'We recommend using useRef() or createRef() instead. ' +
'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' +
Expand Down Expand Up @@ -711,7 +711,7 @@ describe('ReactComponent', () => {
root.render(<Foo />);
});
}).toErrorDev(
'Warning: Functions are not valid as a React child. This may happen if ' +
'Functions are not valid as a React child. This may happen if ' +
'you return Foo instead of <Foo /> from render. ' +
'Or maybe you meant to call this function rather than return it.\n' +
' <Foo>{Foo}</Foo>\n' +
Expand All @@ -733,7 +733,7 @@ describe('ReactComponent', () => {
root.render(<Foo />);
});
}).toErrorDev(
'Warning: Functions are not valid as a React child. This may happen if ' +
'Functions are not valid as a React child. This may happen if ' +
'you return Foo instead of <Foo /> from render. ' +
'Or maybe you meant to call this function rather than return it.\n' +
' <Foo>{Foo}</Foo>\n' +
Expand All @@ -756,7 +756,7 @@ describe('ReactComponent', () => {
root.render(<Foo />);
});
}).toErrorDev(
'Warning: Functions are not valid as a React child. This may happen if ' +
'Functions are not valid as a React child. This may happen if ' +
'you return Foo instead of <Foo /> from render. ' +
'Or maybe you meant to call this function rather than return it.\n' +
' <span>{Foo}</span>\n' +
Expand Down Expand Up @@ -811,13 +811,13 @@ describe('ReactComponent', () => {
root.render(<Foo ref={current => (component = current)} />);
});
}).toErrorDev([
'Warning: Functions are not valid as a React child. This may happen if ' +
'Functions are not valid as a React child. This may happen if ' +
'you return Foo instead of <Foo /> from render. ' +
'Or maybe you meant to call this function rather than return it.\n' +
' <div>{Foo}</div>\n' +
' in div (at **)\n' +
' in Foo (at **)',
'Warning: Functions are not valid as a React child. This may happen if ' +
'Functions are not valid as a React child. This may happen if ' +
'you return Foo instead of <Foo /> from render. ' +
'Or maybe you meant to call this function rather than return it.\n' +
' <span>{Foo}</span>\n' +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ describe('ReactComponentLifeCycle', () => {
root.render(<StatefulComponent />);
});
}).toErrorDev(
"Warning: Can't call setState on a component that is not yet mounted. " +
"Can't call setState on a component that is not yet mounted. " +
'This is a no-op, but it might indicate a bug in your application. ' +
'Instead, assign to `this.state` directly or define a `state = {};` ' +
'class property with the desired state in the StatefulComponent component.',
Expand Down Expand Up @@ -1504,20 +1504,20 @@ describe('ReactComponentLifeCycle', () => {
}).toWarnDev(
[
/* eslint-disable max-len */
`Warning: componentWillMount has been renamed, and is not recommended for use. See https://react.dev/link/unsafe-component-lifecycles for details.
`componentWillMount has been renamed, and is not recommended for use. See https://react.dev/link/unsafe-component-lifecycles for details.
* Move code with side effects to componentDidMount, and set initial state in the constructor.
* Rename componentWillMount to UNSAFE_componentWillMount to suppress this warning in non-strict mode. In React 18.x, only the UNSAFE_ name will work. To rename all deprecated lifecycles to their new names, you can run \`npx react-codemod rename-unsafe-lifecycles\` in your project source folder.
Please update the following components: MyComponent`,
`Warning: componentWillReceiveProps has been renamed, and is not recommended for use. See https://react.dev/link/unsafe-component-lifecycles for details.
`componentWillReceiveProps has been renamed, and is not recommended for use. See https://react.dev/link/unsafe-component-lifecycles for details.
* Move data fetching code or side effects to componentDidUpdate.
* If you're updating state whenever props change, refactor your code to use memoization techniques or move it to static getDerivedStateFromProps. Learn more at: https://react.dev/link/derived-state
* Rename componentWillReceiveProps to UNSAFE_componentWillReceiveProps to suppress this warning in non-strict mode. In React 18.x, only the UNSAFE_ name will work. To rename all deprecated lifecycles to their new names, you can run \`npx react-codemod rename-unsafe-lifecycles\` in your project source folder.
Please update the following components: MyComponent`,
`Warning: componentWillUpdate has been renamed, and is not recommended for use. See https://react.dev/link/unsafe-component-lifecycles for details.
`componentWillUpdate has been renamed, and is not recommended for use. See https://react.dev/link/unsafe-component-lifecycles for details.
* Move data fetching code or side effects to componentDidUpdate.
* Rename componentWillUpdate to UNSAFE_componentWillUpdate to suppress this warning in non-strict mode. In React 18.x, only the UNSAFE_ name will work. To rename all deprecated lifecycles to their new names, you can run \`npx react-codemod rename-unsafe-lifecycles\` in your project source folder.
Expand Down
22 changes: 11 additions & 11 deletions packages/react-dom/src/__tests__/ReactCompositeComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ describe('ReactCompositeComponent', () => {
root.render(<MyComponent />);
});
}).toErrorDev(
"Warning: Can't call forceUpdate on a component that is not yet mounted. " +
"Can't call forceUpdate on a component that is not yet mounted. " +
'This is a no-op, but it might indicate a bug in your application. ' +
'Instead, assign to `this.state` directly or define a `state = {};` ' +
'class property with the desired state in the MyComponent component.',
Expand Down Expand Up @@ -347,7 +347,7 @@ describe('ReactCompositeComponent', () => {
root.render(<MyComponent />);
});
}).toErrorDev(
"Warning: Can't call setState on a component that is not yet mounted. " +
"Can't call setState on a component that is not yet mounted. " +
'This is a no-op, but it might indicate a bug in your application. ' +
'Instead, assign to `this.state` directly or define a `state = {};` ' +
'class property with the desired state in the MyComponent component.',
Expand Down Expand Up @@ -484,7 +484,7 @@ describe('ReactCompositeComponent', () => {
});
}).rejects.toThrow(TypeError);
}).toErrorDev(
'Warning: The <ClassWithRenderNotExtended /> component appears to have a render method, ' +
'The <ClassWithRenderNotExtended /> component appears to have a render method, ' +
"but doesn't extend React.Component. This is likely to cause errors. " +
'Change ClassWithRenderNotExtended to extend React.Component instead.',
);
Expand Down Expand Up @@ -631,7 +631,7 @@ describe('ReactCompositeComponent', () => {
instance.setState({bogus: true});
});
}).toErrorDev(
'Warning: ClassComponent.shouldComponentUpdate(): Returned undefined instead of a ' +
'ClassComponent.shouldComponentUpdate(): Returned undefined instead of a ' +
'boolean value. Make sure to return true or false.',
);
});
Expand All @@ -651,7 +651,7 @@ describe('ReactCompositeComponent', () => {
root.render(<Component />);
});
}).toErrorDev(
'Warning: Component has a method called ' +
'Component has a method called ' +
'componentDidUnmount(). But there is no such lifecycle method. ' +
'Did you mean componentWillUnmount()?',
);
Expand All @@ -673,7 +673,7 @@ describe('ReactCompositeComponent', () => {
root.render(<Component />);
});
}).toErrorDev(
'Warning: Component has a method called ' +
'Component has a method called ' +
'componentDidReceiveProps(). But there is no such lifecycle method. ' +
'If you meant to update the state in response to changing props, ' +
'use componentWillReceiveProps(). If you meant to fetch data or ' +
Expand All @@ -699,7 +699,7 @@ describe('ReactCompositeComponent', () => {
root.render(<Component />);
});
}).toErrorDev(
'Warning: Setting defaultProps as an instance property on Component is not supported ' +
'Setting defaultProps as an instance property on Component is not supported ' +
'and will be ignored. Instead, define defaultProps as a static property on Component.',
);
});
Expand Down Expand Up @@ -1199,9 +1199,9 @@ describe('ReactCompositeComponent', () => {
});
}).rejects.toThrow();
}).toErrorDev([
'Warning: No `render` method found on the RenderTextInvalidConstructor instance: ' +
'No `render` method found on the RenderTextInvalidConstructor instance: ' +
'did you accidentally return an object from the constructor?',
'Warning: No `render` method found on the RenderTextInvalidConstructor instance: ' +
'No `render` method found on the RenderTextInvalidConstructor instance: ' +
'did you accidentally return an object from the constructor?',
]);
});
Expand Down Expand Up @@ -1239,9 +1239,9 @@ describe('ReactCompositeComponent', () => {
});
}).rejects.toThrow();
}).toErrorDev([
'Warning: No `render` method found on the RenderTestUndefinedRender instance: ' +
'No `render` method found on the RenderTestUndefinedRender instance: ' +
'you may have forgotten to define `render`.',
'Warning: No `render` method found on the RenderTestUndefinedRender instance: ' +
'No `render` method found on the RenderTestUndefinedRender instance: ' +
'you may have forgotten to define `render`.',
]);
});
Expand Down
Loading

0 comments on commit 2774208

Please sign in to comment.