Repository: facebook/react
Pull request: [DevTools] Avoid Stringifying Objects during Strict Mode Double Logging

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>

Changed files:
filename: packages/react-devtools-shared/src/__tests__/console-test.js
patch: @@ -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 **)',
+    );
   });
 });
 

filename: packages/react-devtools-shared/src/__tests__/utils-test.js
patch: @@ -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']);
+    });
+  });
 });

filename: packages/react-devtools-shared/src/backend/console.js
patch: @@ -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<string>, 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');
               }

filename: packages/react-devtools-shared/src/backend/utils.js
patch: @@ -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

filename: packages/react-devtools-shared/src/hook.js
patch: @@ -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<any>
-  ): 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<any>,
+    style?: string,
+  ): $ReadOnlyArray<any> {
+    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');
             }