don't stringify objects for console log second render

lunaruan · lunaruan · commit 449744d6e4da · 2022-04-13T18:33:04.000-05:00
diff --git 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,67 @@ 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,
+      ]);
+    });
+  });
 });
diff --git 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';
@@ -335,7 +335,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
@@ -156,6 +156,60 @@ 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 ||
+    (typeof inputArgs[0] === 'string' && inputArgs[0].includes('%c')) ||
+    style === undefined
+  ) {
+    return inputArgs;
+  }
+
+  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
@@ -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');
             }
