From d63cd972454125d4572bb8ffbfeccbdf0c5eb27b Mon Sep 17 00:00:00 2001 From: Luna Ruan Date: Thu, 14 Apr 2022 11:30:04 -0500 Subject: [PATCH] don't stringify objects for console log second render (#24373) 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 --- .../src/__tests__/console-test.js | 67 ++++++++++----- .../src/__tests__/utils-test.js | 82 ++++++++++++++++++- .../src/backend/console.js | 9 +- .../src/backend/utils.js | 56 +++++++++++++ packages/react-devtools-shared/src/hook.js | 80 ++++++++---------- 5 files changed, 223 insertions(+), 71 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/console-test.js b/packages/react-devtools-shared/src/__tests__/console-test.js index 9d69e6c705579..b1f5f68143925 100644 --- a/packages/react-devtools-shared/src/__tests__/console-test.js +++ b/packages/react-devtools-shared/src/__tests__/console-test.js @@ -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', () => { @@ -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', () => { @@ -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 **)', + ); }); }); diff --git a/packages/react-devtools-shared/src/__tests__/utils-test.js b/packages/react-devtools-shared/src/__tests__/utils-test.js index 3ff01c5ef9600..02bc6be4657a7 100644 --- a/packages/react-devtools-shared/src/__tests__/utils-test.js +++ b/packages/react-devtools-shared/src/__tests__/utils-test.js @@ -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, @@ -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']); + }); + }); }); diff --git a/packages/react-devtools-shared/src/backend/console.js b/packages/react-devtools-shared/src/backend/console.js index 95d1f4d103329..5a85b68deac32 100644 --- a/packages/react-devtools-shared/src/backend/console.js +++ b/packages/react-devtools-shared/src/backend/console.js @@ -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'; @@ -38,7 +38,7 @@ const STYLE_DIRECTIVE_REGEX = /^%c/; // so the console color stays consistent function isStrictModeOverride(args: Array, method: string): boolean { return ( - args.length === 2 && + args.length >= 2 && STYLE_DIRECTIVE_REGEX.test(args[0]) && args[1] === `color: ${getConsoleColor(method) || ''}` ); @@ -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); } @@ -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'); } diff --git a/packages/react-devtools-shared/src/backend/utils.js b/packages/react-devtools-shared/src/backend/utils.js index 09f821f4e1877..da911cf94458e 100644 --- a/packages/react-devtools-shared/src/backend/utils.js +++ b/packages/react-devtools-shared/src/backend/utils.js @@ -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, + style?: string, +): $ReadOnlyArray { + 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 diff --git a/packages/react-devtools-shared/src/hook.js b/packages/react-devtools-shared/src/hook.js index 5844074863bf5..1ea54985fab68 100644 --- a/packages/react-devtools-shared/src/hook.js +++ b/packages/react-devtools-shared/src/hook.js @@ -172,54 +172,42 @@ export function installHook(target: any): DevToolsHook | null { } // NOTE: KEEP IN SYNC with src/backend/utils.js - function format( - maybeMessage: any, - ...inputArgs: $ReadOnlyArray - ): string { - const args = inputArgs.slice(); - - // Symbols cannot be concatenated with Strings. - let formatted = String(maybeMessage); - - // If the first argument is a string, check for substitutions. - if (typeof maybeMessage === 'string') { - if (args.length) { - const REGEXP = /(%?)(%([jds]))/g; - - formatted = formatted.replace(REGEXP, (match, escaped, ptn, flag) => { - let arg = args.shift(); - switch (flag) { - case 's': - arg += ''; - break; - case 'd': - case 'i': - arg = parseInt(arg, 10).toString(); - break; - case 'f': - arg = parseFloat(arg).toString(); - break; - } - if (!escaped) { - return arg; - } - args.unshift(arg); - return match; - }); - } + function formatWithStyles( + inputArgs: $ReadOnlyArray, + style?: string, + ): $ReadOnlyArray { + if ( + inputArgs === undefined || + inputArgs === null || + inputArgs.length === 0 || + (typeof inputArgs[0] === 'string' && inputArgs[0].includes('%c')) || + style === undefined + ) { + return inputArgs; } - // Arguments that remain after formatting. - if (args.length) { - for (let i = 0; i < args.length; i++) { - formatted += ' ' + String(args[i]); - } + 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]; } - - // Update escaped %% values. - formatted = formatted.replace(/%{2,2}/g, '%'); - - return String(formatted); } let unpatchFn = null; @@ -291,7 +279,7 @@ export function installHook(target: any): DevToolsHook | null { } if (color) { - originalMethod(`%c${format(...args)}`, `color: ${color}`); + originalMethod(...formatWithStyles(args, `color: ${color}`)); } else { throw Error('Console color is not defined'); }