Skip to content

Commit

Permalink
don't stringify objects for console log second render (#24373)
Browse files Browse the repository at this point in the history 
Fixes #24302 based on #24306.
---

The current implementation for strict mode double logging stringiness and dims the second log. However, because we stringify everything, including objects, this causes objects to be logged as `[object Object]` etc.

This PR creates a new function that formats console log arguments with a specified style. It does this by:
1. The first param is a string that contains %c: Bail out and return the args without modifying the styles. We don't want to affect styles that the developer deliberately set.
2. The first param is a string that doesn't contain %c but contains string formatting: `[`%c${args[0]}`, style, ...args.slice(1)]` Note: we assume that the string formatting that the developer uses is correct.
3. The first param is a string that doesn't contain string formatting OR is not a string: Create a formatting string where:
   -  boolean, string, symbol -> %s
   -  number -> %f OR %i depending on if it's an int or float
   -  default -> %o
---
Co-authored-by: Billy Janitsch <billy@kensho.com>
  • Loading branch information
@lunaruan
lunaruan committed Apr 14, 2022
1 parent ddb1ab1 commit d63cd97
Show file tree
Hide file tree
Showing 5 changed files with 223 additions and 71 deletions.
67 changes: 47 additions & 20 deletions packages/react-devtools-shared/src/__tests__/console-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -494,20 +494,31 @@ describe('console', () => {
);
expect(mockLog.mock.calls[0]).toHaveLength(1);
expect(mockLog.mock.calls[0][0]).toBe('log');
expect(mockLog.mock.calls[1]).toHaveLength(2);
expect(mockLog.mock.calls[1][0]).toBe('%clog');
expect(mockLog.mock.calls[1]).toEqual([
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_LOG_COLOR}`,
'log',
]);

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

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

it('should not double log if hideConsoleLogsInStrictMode is enabled in Strict mode', () => {
Expand Down Expand Up @@ -577,20 +588,32 @@ describe('console', () => {
expect(mockLog).toHaveBeenCalledTimes(2);
expect(mockLog.mock.calls[0]).toHaveLength(1);
expect(mockLog.mock.calls[0][0]).toBe('log');
expect(mockLog.mock.calls[1]).toHaveLength(2);
expect(mockLog.mock.calls[1][0]).toBe('%clog');
expect(mockLog.mock.calls[1]).toHaveLength(3);
expect(mockLog.mock.calls[1]).toEqual([
'%c%s',
`color: ${process.env.DARK_MODE_DIMMED_LOG_COLOR}`,
'log',
]);

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

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

it('should not double log in Strict mode initial render for extension', () => {
Expand Down Expand Up @@ -666,22 +689,26 @@ describe('console', () => {
expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockWarn.mock.calls[1]).toHaveLength(2);
expect(normalizeCodeLocInfo(mockWarn.mock.calls[1][0])).toEqual(
'%cwarn \n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockWarn.mock.calls[1]).toHaveLength(4);
expect(mockWarn.mock.calls[1][0]).toEqual('%c%s %s');
expect(mockWarn.mock.calls[1][1]).toMatch('color: rgba(');
expect(mockWarn.mock.calls[1][2]).toEqual('warn');
expect(normalizeCodeLocInfo(mockWarn.mock.calls[1][3]).trim()).toEqual(
'in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);

expect(mockError).toHaveBeenCalledTimes(2);
expect(mockError.mock.calls[0]).toHaveLength(2);
expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toEqual(
'\n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockError.mock.calls[1]).toHaveLength(2);
expect(normalizeCodeLocInfo(mockError.mock.calls[1][0])).toEqual(
'%cerror \n in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
expect(mockError.mock.calls[1]).toHaveLength(4);
expect(mockError.mock.calls[1][0]).toEqual('%c%s %s');
expect(mockError.mock.calls[1][1]).toMatch('color: rgba(');
expect(mockError.mock.calls[1][2]).toEqual('error');
expect(normalizeCodeLocInfo(mockError.mock.calls[1][3]).trim()).toEqual(
'in Child (at **)\n in Intermediate (at **)\n in Parent (at **)',
);
});
});

Expand Down
82 changes: 81 additions & 1 deletion packages/react-devtools-shared/src/__tests__/utils-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ import {
getDisplayName,
getDisplayNameForReactElement,
} from 'react-devtools-shared/src/utils';
import {format} from 'react-devtools-shared/src/backend/utils';
import {
format,
formatWithStyles,
} from 'react-devtools-shared/src/backend/utils';
import {
REACT_SUSPENSE_LIST_TYPE as SuspenseList,
REACT_STRICT_MODE_TYPE as StrictMode,
Expand Down Expand Up @@ -110,4 +113,81 @@ describe('utils', () => {
expect(format(Symbol('abc'), 123)).toEqual('Symbol(abc) 123');
});
});

describe('formatWithStyles', () => {
it('should format empty arrays', () => {
expect(formatWithStyles([])).toEqual([]);
expect(formatWithStyles([], 'gray')).toEqual([]);
expect(formatWithStyles(undefined)).toEqual(undefined);
});

it('should bail out of strings with styles', () => {
expect(
formatWithStyles(['%ca', 'color: green', 'b', 'c'], 'color: gray'),
).toEqual(['%ca', 'color: green', 'b', 'c']);
});

it('should format simple strings', () => {
expect(formatWithStyles(['a'])).toEqual(['a']);

expect(formatWithStyles(['a', 'b', 'c'])).toEqual(['a', 'b', 'c']);
expect(formatWithStyles(['a'], 'color: gray')).toEqual([
'%c%s',
'color: gray',
'a',
]);
expect(formatWithStyles(['a', 'b', 'c'], 'color: gray')).toEqual([
'%c%s %s %s',
'color: gray',
'a',
'b',
'c',
]);
});

it('should format string substituions', () => {
expect(
formatWithStyles(['%s %s %s', 'a', 'b', 'c'], 'color: gray'),
).toEqual(['%c%s %s %s', 'color: gray', 'a', 'b', 'c']);

// The last letter isn't gray here but I think it's not a big
// deal, since there is a string substituion but it's incorrect
expect(
formatWithStyles(['%s %s', 'a', 'b', 'c'], 'color: gray'),
).toEqual(['%c%s %s', 'color: gray', 'a', 'b', 'c']);
});

it('should support multiple argument types', () => {
const symbol = Symbol('a');
expect(
formatWithStyles(
['abc', 123, 12.3, true, {hello: 'world'}, symbol],
'color: gray',
),
).toEqual([
'%c%s %i %f %s %o %s',
'color: gray',
'abc',
123,
12.3,
true,
{hello: 'world'},
symbol,
]);
});

it('should properly format escaped string substituions', () => {
expect(formatWithStyles(['%%s'], 'color: gray')).toEqual([
'%c%s',
'color: gray',
'%%s',
]);
expect(formatWithStyles(['%%c'], 'color: gray')).toEqual([
'%c%s',
'color: gray',
'%%c',
]);
expect(formatWithStyles(['%%c%c'], 'color: gray')).toEqual(['%%c%c']);
});
});
});
9 changes: 5 additions & 4 deletions packages/react-devtools-shared/src/backend/console.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import type {Fiber} from 'react-reconciler/src/ReactInternalTypes';
import type {CurrentDispatcherRef, ReactRenderer, WorkTagMap} from './types';
import type {BrowserTheme} from 'react-devtools-shared/src/devtools/views/DevTools';
import {format} from './utils';
import {format, formatWithStyles} from './utils';

import {getInternalReactConstants} from './renderer';
import {getStackByFiberInDevAndProd} from './DevToolsFiberComponentStack';
Expand Down Expand Up @@ -38,7 +38,7 @@ const STYLE_DIRECTIVE_REGEX = /^%c/;
// so the console color stays consistent
function isStrictModeOverride(args: Array<string>, method: string): boolean {
return (
args.length === 2 &&
args.length >= 2 &&
STYLE_DIRECTIVE_REGEX.test(args[0]) &&
args[1] === `color: ${getConsoleColor(method) || ''}`
);
Expand Down Expand Up @@ -246,7 +246,8 @@ export function patch({
);
if (componentStack !== '') {
if (isStrictModeOverride(args, method)) {
args[0] = format(args[0], componentStack);
args[0] = `${args[0]} %s`;
args.push(componentStack);
} else {
args.push(componentStack);
}
Expand Down Expand Up @@ -335,7 +336,7 @@ export function patchForStrictMode() {
} else {
const color = getConsoleColor(method);
if (color) {
originalMethod(`%c${format(...args)}`, `color: ${color}`);
originalMethod(...formatWithStyles(args, `color: ${color}`));
} else {
throw Error('Console color is not defined');
}
Expand Down
56 changes: 56 additions & 0 deletions packages/react-devtools-shared/src/backend/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,62 @@ export function serializeToString(data: any): string {
});
}

// Formats an array of args with a style for console methods, using
// the following algorithm:
// 1. The first param is a string that contains %c
// - Bail out and return the args without modifying the styles.
// We don't want to affect styles that the developer deliberately set.
// 2. The first param is a string that doesn't contain %c but contains
// string formatting
// - [`%c${args[0]}`, style, ...args.slice(1)]
// - Note: we assume that the string formatting that the developer uses
// is correct.
// 3. The first param is a string that doesn't contain string formatting
// OR is not a string
// - Create a formatting string where:
// boolean, string, symbol -> %s
// number -> %f OR %i depending on if it's an int or float
// default -> %o
export function formatWithStyles(
inputArgs: $ReadOnlyArray<any>,
style?: string,
): $ReadOnlyArray<any> {
if (
inputArgs === undefined ||
inputArgs === null ||
inputArgs.length === 0 ||
// Matches any of %c but not %%c
(typeof inputArgs[0] === 'string' && inputArgs[0].match(/([^%]|^)(%c)/g)) ||
style === undefined
) {
return inputArgs;
}

// Matches any of %(o|O|i|s|f), but not %%(o|O|i|s|f)
const REGEXP = /([^%]|^)(%([oOdisf]))/g;
if (inputArgs[0].match(REGEXP)) {
return [`%c${inputArgs[0]}`, style, ...inputArgs.slice(1)];
} else {
const firstArg = inputArgs.reduce((formatStr, elem, i) => {
if (i > 0) {
formatStr += ' ';
}
switch (typeof elem) {
case 'string':
case 'boolean':
case 'symbol':
return (formatStr += '%s');
case 'number':
const formatting = Number.isInteger(elem) ? '%i' : '%f';
return (formatStr += formatting);
default:
return (formatStr += '%o');
}
}, '%c');
return [firstArg, style, ...inputArgs];
}
}

// based on https://github.com/tmpfs/format-util/blob/0e62d430efb0a1c51448709abd3e2406c14d8401/format.js#L1
// based on https://developer.mozilla.org/en-US/docs/Web/API/console#Using_string_substitutions
// Implements s, d, i and f placeholders
Expand Down

0 comments on commit d63cd97

Please sign in to comment.