Add disallowStringConstructor option to lint rule

This commit adds a new option to the react-internal/safe-string-coercion
lint rule that controls whether `String(value)` should be disallowed.

ESLint is now configured set up to disallow `String(value)` in
perf-sensitive production code files, and to allow it elsewhere.

Although this option is enabled for prod code, error-handling
code should use `String()` for safety when building error messages
and call stacks. There were 5 lines where `String()` was used for prod
error handling. Tthis commit disables the lint rule for these lines.

Author: justingrant

.eslintrc.js

@@ -108,7 +108,10 @@ module.exports = {
     // CUSTOM RULES
     // the second argument of warning/invariant should be a literal string
     'react-internal/no-primitive-constructors': ERROR,
-    'react-internal/safe-string-coercion': ERROR,
+    'react-internal/safe-string-coercion': [
+      ERROR,
+      {disallowStringConstructor: true},
+    ],
     'react-internal/no-to-warn-dev-within-to-throw': ERROR,
     'react-internal/invariant-args': ERROR,
     'react-internal/warning-args': ERROR,
@@ -169,10 +172,17 @@ module.exports = {
         'packages/*/npm/**/*.js',
         'packages/dom-event-testing-library/**/*.js',
         'packages/react-devtools*/**/*.js',
+        'dangerfile.js',
+        'fixtures',
+        'packages/react-dom/src/test-utils/*.js',
       ],
       rules: {
         'react-internal/no-production-logging': OFF,
         'react-internal/warning-args': OFF,
+        'react-internal/safe-string-coercion': [
+          ERROR,
+          {disallowStringConstructor: false},
+        ],
 
         // Disable accessibility checks
         'jsx-a11y/aria-role': OFF,

packages/react-server/src/ReactFlightServer.js

@@ -296,7 +296,7 @@ function describeValueForErrorMessage(value: ReactModel): string {
     case 'function':
       return 'function';
     default:
-      // eslint-disable-next-line
+      // eslint-disable-next-line react-internal/safe-string-coercion
       return String(value);
   }
 }
@@ -616,7 +616,9 @@ function emitErrorChunk(request: Request, id: number, error: mixed): void {
   let stack = '';
   try {
     if (error instanceof Error) {
+      // eslint-disable-next-line react-internal/safe-string-coercion
       message = String(error.message);
+      // eslint-disable-next-line react-internal/safe-string-coercion
       stack = String(error.stack);
     } else {
       message = 'Error: ' + (error: any);

packages/react/src/ReactChildren.js

@@ -202,6 +202,7 @@ function mapIntoArray(
         );
       }
     } else if (type === 'object') {
+      // eslint-disable-next-line react-internal/safe-string-coercion
       const childrenString = String((children: any));
       invariant(
         false,

packages/shared/consoleWithStackDev.js

@@ -47,6 +47,7 @@ function printWarning(level, format, args) {
       args = args.concat([stack]);
     }
 
+    // eslint-disable-next-line react-internal/safe-string-coercion
     const argsWithFormat = args.map(item => String(item));
     // Careful: RN currently depends on this prefix
     argsWithFormat.unshift('Warning: ' + format);

scripts/eslint-rules/__tests__/safe-string-coercion-test.internal.js

@@ -14,7 +14,14 @@ const {RuleTester} = require('eslint');
 const ruleTester = new RuleTester();
 
 ruleTester.run('eslint-rules/safe-string-coercion', rule, {
-  valid: ['String(obj)', "'a' + obj"],
+  valid: [
+    {
+      code: 'String(obj)',
+      options: [{disallowStringConstructor: false}],
+    },
+    'String(obj)',
+    "'a' + obj",
+  ],
   invalid: [
     {
       code: "'' + obj",
@@ -40,5 +47,18 @@ ruleTester.run('eslint-rules/safe-string-coercion', rule, {
         },
       ],
     },
+    {
+      code: 'String(obj)',
+      options: [{disallowStringConstructor: true}],
+      errors: [
+        {
+          message:
+            "For perf-sensitive coercion, avoid `String(value)`. Instead, use `'' + value`." +
+            ' Precede it with a DEV check from shared/CheckStringCoercion' +
+            ' unless Symbol and Temporal.* values are impossible.' +
+            ' For non-prod code and prod error handling, use `String(value)` and disable this rule.',
+        },
+      ],
+    },
   ],
 });

scripts/eslint-rules/safe-string-coercion.js

@@ -17,9 +17,41 @@ function isEmptyLiteral(node) {
   );
 }
 
+// Symbols and Temporal.* objects will throw when using `'' + value`, but that
+// pattern can be faster than `String(value)` because JS engines can optimize
+// `+` better in some cases. Therefore, in perf-sensitive production codepaths
+// we require using `'' + value` for string coercion. The only exception is prod
+// error handling code, because it's bad to crash while assembling an error
+// message or call stack! Also, error-handling code isn't usually perf-critical.
+//
+// Non-production codepaths (tests, devtools extension, build tools, etc.)
+// should use `String(value)` because it will never crash and the (small) perf
+// difference doesn't matter enough for non-prod use cases.
+//
+// This rule assists enforcing these guidelines:
+// * `'' + value` is flagged with a message to remind developers to add a DEV
+//   check from shared/CheckStringCoercion.js to make sure that the user gets a
+//   clear error message in DEV is the coercion will throw. These checks are not
+//   needed if throwing is not possible, e.g. if the value is already known to
+//   be a string or number.
+// * `String(value)` is flagged only if the `disallowStringConstructor` option
+//   is set. Set this option for prod code files, and don't set it for non-prod
+//   files.
+
 module.exports = {
   meta: {
-    schema: [],
+    schema: [
+      {
+        type: 'object',
+        properties: {
+          disallowStringConstructor: {
+            type: 'boolean',
+            default: false,
+          },
+        },
+        additionalProperties: false,
+      },
+    ],
   },
   create(context) {
     return {
@@ -38,6 +70,19 @@ module.exports = {
           return;
         }
       },
+      CallExpression: function(node) {
+        const disallowStringConstructor =
+          context.options[0] && context.options[0].disallowStringConstructor;
+        if (disallowStringConstructor && node.callee.name === 'String') {
+          context.report(
+            node,
+            "For perf-sensitive coercion, avoid `String(value)`. Instead, use `'' + value`." +
+              ' Precede it with a DEV check from shared/CheckStringCoercion' +
+              ' unless Symbol and Temporal.* values are impossible.' +
+              ' For non-prod code and prod error handling, use `String(value)` and disable this rule.'
+          );
+        }
+      },
     };
   },
 };