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.

Sign up for GitHub

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

Jump to bottom

React Native fixes for new inline errors feature #20502

Merged
merged 2 commits into from
Dec 22, 2020
Merged

React Native fixes for new inline errors feature #20502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24a2098
Remove component stack removal logic. This was fragile and broke RN w…
Dec 22, 2020
384936b
Default inline errors/warnings to on in the backend
Dec 22, 2020
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
3 changes: 2 additions & 1 deletion packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2014,7 +2014,8 @@ describe('InspectedElementContext', () => {
Object {
"errors": Array [
Array [
"Warning: Each child in a list should have a unique \\"key\\" prop. See https://reactjs.org/link/warning-keys for more information.",
"Warning: Each child in a list should have a unique \\"key\\" prop. See https://reactjs.org/link/warning-keys for more information.
at Example",
1,
],
],
Expand Down
13 changes: 5 additions & 8 deletions packages/react-devtools-shared/src/backend/console.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,12 +140,12 @@ export function patch({
targetConsole[method]);

const overrideMethod = (...args) => {
const lastArg = args.length > 0 ? args[args.length - 1] : null;
const alreadyHasComponentStack =
lastArg !== null && isStringComponentStack(lastArg);

let shouldAppendWarningStack = false;
if (consoleSettingsRef.appendComponentStack) {
const lastArg = args.length > 0 ? args[args.length - 1] : null;
const alreadyHasComponentStack =
lastArg !== null && isStringComponentStack(lastArg);

// If we are ever called with a string that already has a component stack,
// e.g. a React error/warning, don't append a second stack.
shouldAppendWarningStack = !alreadyHasComponentStack;
Expand Down Expand Up @@ -176,10 +176,7 @@ export function patch({
current,
((method: any): 'error' | 'warn'),
// Copy args before we mutate them (e.g. adding the component stack)
alreadyHasComponentStack
? // Replace component stack with an empty string in case there's a string placeholder for it.
[...args.slice(0, -1), '']
: args.slice(),
args.slice(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caused problems for RN+LogBox which did not pass component stack in as an additional parameter but instead as part of the original string. In hindsight, it's better for us just to not be clever here and pass through the string as-is.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. It made more sense when we wrapped the error message. Now that we truncate (and most error messages are truncated) it's not that "bad" to display too much information.

);
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/backend/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,7 +634,7 @@ export function attach(
const breakOnConsoleErrors =
window.__REACT_DEVTOOLS_BREAK_ON_CONSOLE_ERRORS__ === true;
const showInlineWarningsAndErrors =
window.__REACT_DEVTOOLS_SHOW_INLINE_WARNINGS_AND_ERRORS__ === true;
window.__REACT_DEVTOOLS_SHOW_INLINE_WARNINGS_AND_ERRORS__ !== false;
if (appendComponentStack || breakOnConsoleErrors) {
patchConsole({
appendComponentStack,
Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/hook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ export function installHook(target: any): DevToolsHook | null {
const breakOnConsoleErrors =
window.__REACT_DEVTOOLS_BREAK_ON_CONSOLE_ERRORS__ === true;
const showInlineWarningsAndErrors =
window.__REACT_DEVTOOLS_SHOW_INLINE_WARNINGS_AND_ERRORS__ === true;
window.__REACT_DEVTOOLS_SHOW_INLINE_WARNINGS_AND_ERRORS__ !== false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have been on by default here, to match the frontend UI setting. As a bonus this fixes RN startup use case as well.


// The installHook() function is injected by being stringified in the browser,
// so imports outside of this function do not get included.
Expand Down