Skip to content

[react devtools] Don't check for NODE_ENV==='test' because it never is #25186

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

Merged
merged 1 commit into from
Sep 9, 2022
Merged

[react devtools] Don't check for NODE_ENV==='test' because it never is #25186

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
48 changes: 22 additions & 26 deletions 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 @@ -812,32 +812,28 @@ export function attach(
// Patching the console enables DevTools to do a few useful things:
// * Append component stacks to warnings and error messages
// * Disable logging during re-renders to inspect hooks (see inspectHooksOfFiber)
//
// Don't patch in test environments because we don't want to interfere with Jest's own console overrides.
if (process.env.NODE_ENV !== 'test') {
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 condition never evaluates to true

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good catch. We probably want to do if (!__TEST__) here instead

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 breaks these tests:

Test Suites: 7 failed, 27 passed, 34 total
Tests:       49 failed, 6 skipped, 383 passed, 438 total
Snapshots:   97 failed, 566 passed, 663 total

I haven't looked into why, but it looks like many tests assume that registerRendererWithConsole(renderer); will be called. Do you have any thoughts on whether we want registerRendererWithConsole to be called here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it should be OK to move registerRendererWithConsole out of the test because we're not actually overriding the console at all there, just adding some functions for the console override methods to use

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like wrapping any section in if (!__TEST__) breaks at least that many tests. Thoughts on landing this as is?

Copy link
Contributor

@lunaruan lunaruan Sep 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Sure do it :) Curious to know the tests that are breaking though, and why. Do you think you could figure that out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think when I explore making changes to not use window.__REACT_DEVTOOLS_GLOBAL_FOO__-type variables, I should be able to figure it out

registerRendererWithConsole(renderer, onErrorOrWarning);

// The renderer interface can't read these preferences directly,
// because it is stored in localStorage within the context of the extension.
// It relies on the extension to pass the preference through via the global.
const appendComponentStack =
window.__REACT_DEVTOOLS_APPEND_COMPONENT_STACK__ !== false;
const breakOnConsoleErrors =
window.__REACT_DEVTOOLS_BREAK_ON_CONSOLE_ERRORS__ === true;
const showInlineWarningsAndErrors =
window.__REACT_DEVTOOLS_SHOW_INLINE_WARNINGS_AND_ERRORS__ !== false;
const hideConsoleLogsInStrictMode =
window.__REACT_DEVTOOLS_HIDE_CONSOLE_LOGS_IN_STRICT_MODE__ === true;
const browserTheme = window.__REACT_DEVTOOLS_BROWSER_THEME__;

patchConsole({
appendComponentStack,
breakOnConsoleErrors,
showInlineWarningsAndErrors,
hideConsoleLogsInStrictMode,
browserTheme,
});
}
registerRendererWithConsole(renderer, onErrorOrWarning);

// The renderer interface can't read these preferences directly,
// because it is stored in localStorage within the context of the extension.
// It relies on the extension to pass the preference through via the global.
const appendComponentStack =
window.__REACT_DEVTOOLS_APPEND_COMPONENT_STACK__ !== false;
const breakOnConsoleErrors =
window.__REACT_DEVTOOLS_BREAK_ON_CONSOLE_ERRORS__ === true;
const showInlineWarningsAndErrors =
window.__REACT_DEVTOOLS_SHOW_INLINE_WARNINGS_AND_ERRORS__ !== false;
const hideConsoleLogsInStrictMode =
window.__REACT_DEVTOOLS_HIDE_CONSOLE_LOGS_IN_STRICT_MODE__ === true;
const browserTheme = window.__REACT_DEVTOOLS_BROWSER_THEME__;

patchConsole({
appendComponentStack,
breakOnConsoleErrors,
showInlineWarningsAndErrors,
hideConsoleLogsInStrictMode,
browserTheme,
});

const debug = (
name: string,
Expand Down