Skip to content

Commit

Permalink
Handle info, group, and groupCollapsed in Strict Mode logging (#25172)
Browse files Browse the repository at this point in the history
* Handle info, group, and groupCollapsed in Strict Mode logging

While working on the new Next.js router which heavily relies on useReducer I noticed that `group` and `groupCollapsed` which both take labels were showing as-is in the console for the second render/dispatch in Strict Mode logs. While looking at the code I found that `info` was also not instrumented.

I've added additional handling for:
- `info`
- `group`
- `groupCollapsed`

* Remove console.log

* Fix tests
  • Loading branch information
timneutkens committed Sep 6, 2022
1 parent 81b79c7 commit a9dc73c
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 2 deletions.
65 changes: 65 additions & 0 deletions packages/react-devtools-shared/src/__tests__/console-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ let fakeConsole;
let legacyRender;
let mockError;
let mockInfo;
let mockGroup;
let mockGroupCollapsed;
let mockLog;
let mockWarn;
let patchConsole;
Expand All @@ -25,6 +27,7 @@ let rendererID;
describe('console', () => {
beforeEach(() => {
const Console = require('react-devtools-shared/src/backend/console');

patchConsole = Console.patch;
unpatchConsole = Console.unpatch;

Expand All @@ -33,13 +36,17 @@ describe('console', () => {
// because Jest itself has hooks into it as does our test env setup.
mockError = jest.fn();
mockInfo = jest.fn();
mockGroup = jest.fn();
mockGroupCollapsed = jest.fn();
mockLog = jest.fn();
mockWarn = jest.fn();
fakeConsole = {
error: mockError,
info: mockInfo,
log: mockLog,
warn: mockWarn,
group: mockGroup,
groupCollapsed: mockGroupCollapsed,
};

Console.dangerous_setTargetConsoleForTesting(fakeConsole);
Expand Down Expand Up @@ -69,6 +76,8 @@ describe('console', () => {
expect(fakeConsole.info).toBe(mockInfo);
expect(fakeConsole.log).toBe(mockLog);
expect(fakeConsole.warn).not.toBe(mockWarn);
expect(fakeConsole.group).toBe(mockGroup);
expect(fakeConsole.groupCollapsed).toBe(mockGroupCollapsed);
});

// @reactVersion >=18.0
Expand Down Expand Up @@ -491,6 +500,9 @@ describe('console', () => {
fakeConsole.log('log');
fakeConsole.warn('warn');
fakeConsole.error('error');
fakeConsole.info('info');
fakeConsole.group('group');
fakeConsole.groupCollapsed('groupCollapsed');
return <div />;
}

Expand Down Expand Up @@ -528,6 +540,36 @@ describe('console', () => {
`color: ${process.env.DARK_MODE_DIMMED_ERROR_COLOR}`,
'error',
]);

expect(mockInfo).toHaveBeenCalledTimes(2);
expect(mockInfo.mock.calls[0]).toHaveLength(1);
expect(mockInfo.mock.calls[0][0]).toBe('info');
expect(mockInfo.mock.calls[1]).toHaveLength(3);
expect(mockInfo.mock.calls[1]).toEqual([
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_LOG_COLOR}`,
'info',
]);

expect(mockGroup).toHaveBeenCalledTimes(2);
expect(mockGroup.mock.calls[0]).toHaveLength(1);
expect(mockGroup.mock.calls[0][0]).toBe('group');
expect(mockGroup.mock.calls[1]).toHaveLength(3);
expect(mockGroup.mock.calls[1]).toEqual([
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_LOG_COLOR}`,
'group',
]);

expect(mockGroupCollapsed).toHaveBeenCalledTimes(2);
expect(mockGroupCollapsed.mock.calls[0]).toHaveLength(1);
expect(mockGroupCollapsed.mock.calls[0][0]).toBe('groupCollapsed');
expect(mockGroupCollapsed.mock.calls[1]).toHaveLength(3);
expect(mockGroupCollapsed.mock.calls[1]).toEqual([
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_LOG_COLOR}`,
'groupCollapsed',
]);
});

it('should not double log if hideConsoleLogsInStrictMode is enabled in Strict mode', () => {
Expand All @@ -538,9 +580,16 @@ describe('console', () => {
const root = ReactDOMClient.createRoot(container);

function App() {
console.log(
'CALL',
global.__REACT_DEVTOOLS_HIDE_CONSOLE_LOGS_IN_STRICT_MODE__,
);
fakeConsole.log('log');
fakeConsole.warn('warn');
fakeConsole.error('error');
fakeConsole.info('info');
fakeConsole.group('group');
fakeConsole.groupCollapsed('groupCollapsed');
return <div />;
}

Expand All @@ -563,6 +612,18 @@ describe('console', () => {
expect(mockError).toHaveBeenCalledTimes(1);
expect(mockError.mock.calls[0]).toHaveLength(1);
expect(mockError.mock.calls[0][0]).toBe('error');

expect(mockInfo).toHaveBeenCalledTimes(1);
expect(mockInfo.mock.calls[0]).toHaveLength(1);
expect(mockInfo.mock.calls[0][0]).toBe('info');

expect(mockGroup).toHaveBeenCalledTimes(1);
expect(mockGroup.mock.calls[0]).toHaveLength(1);
expect(mockGroup.mock.calls[0][0]).toBe('group');

expect(mockGroupCollapsed).toHaveBeenCalledTimes(1);
expect(mockGroupCollapsed.mock.calls[0]).toHaveLength(1);
expect(mockGroupCollapsed.mock.calls[0][0]).toBe('groupCollapsed');
});

it('should double log in Strict mode initial render for extension', () => {
Expand Down Expand Up @@ -734,13 +795,17 @@ describe('console error', () => {
// because Jest itself has hooks into it as does our test env setup.
mockError = jest.fn();
mockInfo = jest.fn();
mockGroup = jest.fn();
mockGroupCollapsed = jest.fn();
mockLog = jest.fn();
mockWarn = jest.fn();
fakeConsole = {
error: mockError,
info: mockInfo,
log: mockLog,
warn: mockWarn,
group: mockGroup,
groupCollapsed: mockGroupCollapsed,
};

Console.dangerous_setTargetConsoleForTesting(fakeConsole);
Expand Down
10 changes: 9 additions & 1 deletion packages/react-devtools-shared/src/backend/console.js
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,15 @@ let unpatchForStrictModeFn: null | (() => void) = null;
// NOTE: KEEP IN SYNC with src/hook.js:patchConsoleForInitialRenderInStrictMode
export function patchForStrictMode() {
if (consoleManagedByDevToolsDuringStrictMode) {
const overrideConsoleMethods = ['error', 'trace', 'warn', 'log'];
const overrideConsoleMethods = [
'error',
'group',
'groupCollapsed',
'info',
'log',
'trace',
'warn',
];

if (unpatchForStrictModeFn !== null) {
// Don't patch twice.
Expand Down
10 changes: 9 additions & 1 deletion packages/react-devtools-shared/src/hook.js
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,15 @@ export function installHook(target: any): DevToolsHook | null {
hideConsoleLogsInStrictMode: boolean,
browserTheme: BrowserTheme,
}) {
const overrideConsoleMethods = ['error', 'trace', 'warn', 'log'];
const overrideConsoleMethods = [
'error',
'group',
'groupCollapsed',
'info',
'log',
'trace',
'warn',
];

if (unpatchFn !== null) {
// Don't patch twice.
Expand Down

0 comments on commit a9dc73c

Please sign in to comment.