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

[DevTools] Avoid Stringifying Objects during Strict Mode Double Logging #24373

Merged
merged 1 commit into from
Apr 14, 2022
Merged

[DevTools] Avoid Stringifying Objects during Strict Mode Double Logging #24373

Show file tree
Hide file tree
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
67 changes: 47 additions & 20 deletions packages/react-devtools-shared/src/__tests__/console-test.js
View file
Open in desktop
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
Open in desktop
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']);
});
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure you're aware of this, but it looks like this change broke some pre-existing tests in CI:
https://app.circleci.com/pipelines/github/facebook/react/26207/workflows/06a6279f-0816-4dfc-81c5-b2f9228474c1/jobs/471951

9 changes: 5 additions & 4 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 @@ -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
Open in desktop
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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I’m not sure checking for %% this way is quite what you want – eg %%%s is an escaped % followed by a string value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh good call

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
Loading