Fixed another Symbol concatenation issue with DevTools format() util (f…

…acebook#21521)
koto · Jun 15, 2021 · e0f8333 · e0f8333
1 parent fec2b5e
commit e0f8333
Show file tree

Hide file tree

Showing 2 changed files with 71 additions and 30 deletions.
diff --git a/packages/react-devtools-shared/src/__tests__/utils-test.js b/packages/react-devtools-shared/src/__tests__/utils-test.js
@@ -11,6 +11,7 @@ import {
   getDisplayName,
   getDisplayNameForReactElement,
 } from 'react-devtools-shared/src/utils';
+import {format} from 'react-devtools-shared/src/backend/utils';
 import {
   REACT_SUSPENSE_LIST_TYPE as SuspenseList,
   REACT_STRICT_MODE_TYPE as StrictMode,
@@ -45,6 +46,7 @@ describe('utils', () => {
       expect(getDisplayName(FauxComponent, 'Fallback')).toEqual('Fallback');
     });
   });
+
   describe('getDisplayNameForReactElement', () => {
     it('should return correct display name for an element with function type', () => {
       function FauxComponent() {}
@@ -54,29 +56,58 @@ describe('utils', () => {
         'OverrideDisplayName',
       );
     });
+
     it('should return correct display name for an element with a type of StrictMode', () => {
       const element = createElement(StrictMode);
       expect(getDisplayNameForReactElement(element)).toEqual('StrictMode');
     });
+
     it('should return correct display name for an element with a type of SuspenseList', () => {
       const element = createElement(SuspenseList);
       expect(getDisplayNameForReactElement(element)).toEqual('SuspenseList');
     });
+
     it('should return NotImplementedInDevtools for an element with invalid symbol type', () => {
       const element = createElement(Symbol('foo'));
       expect(getDisplayNameForReactElement(element)).toEqual(
         'NotImplementedInDevtools',
       );
     });
+
     it('should return NotImplementedInDevtools for an element with invalid type', () => {
       const element = createElement(true);
       expect(getDisplayNameForReactElement(element)).toEqual(
         'NotImplementedInDevtools',
       );
     });
+
     it('should return Element for null type', () => {
       const element = createElement();
       expect(getDisplayNameForReactElement(element)).toEqual('Element');
     });
   });
+
+  describe('format', () => {
+    it('should format simple strings', () => {
+      expect(format('a', 'b', 'c')).toEqual('a b c');
+    });
+
+    it('should format multiple argument types', () => {
+      expect(format('abc', 123, true)).toEqual('abc 123 true');
+    });
+
+    it('should support string substitutions', () => {
+      expect(format('a %s b %s c', 123, true)).toEqual('a 123 b true c');
+    });
+
+    it('should gracefully handle Symbol types', () => {
+      expect(format(Symbol('a'), 'b', Symbol('c'))).toEqual(
+        'Symbol(a) b Symbol(c)',
+      );
+    });
+
+    it('should gracefully handle Symbol type for the first argument', () => {
+      expect(format(Symbol('abc'), 123)).toEqual('Symbol(abc) 123');
+    });
+  });
 });
diff --git a/packages/react-devtools-shared/src/backend/utils.js b/packages/react-devtools-shared/src/backend/utils.js
@@ -163,43 +163,53 @@ export function format(
   maybeMessage: any,
   ...inputArgs: $ReadOnlyArray<any>
 ): string {
-  if (typeof maybeMessage !== 'string') {
-    return [maybeMessage, ...inputArgs].join(' ');
-  }
-
-  const re = /(%?)(%([jds]))/g;
   const args = inputArgs.slice();
-  let formatted: string = maybeMessage;
 
-  if (args.length) {
-    formatted = formatted.replace(re, (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;
-    });
+  // Symbols cannot be concatenated with Strings.
+  let formatted: string =
+    typeof maybeMessage === 'symbol'
+      ? maybeMessage.toString()
+      : '' + 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;
+      });
+    }
   }
 
-  // arguments remain after formatting
+  // Arguments that remain after formatting.
   if (args.length) {
-    formatted += ' ' + args.join(' ');
+    for (let i = 0; i < args.length; i++) {
+      const arg = args[i];
+
+      // Symbols cannot be concatenated with Strings.
+      formatted += ' ' + (typeof arg === 'symbol' ? arg.toString() : arg);
+    }
   }
 
-  // update escaped %% values
+  // Update escaped %% values.
   formatted = formatted.replace(/%{2,2}/g, '%');
 
   return '' + formatted;