From 475f9b3213e1ffb188c1533b01ae91569812fc08 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sat, 25 Sep 2021 13:27:06 -0400 Subject: [PATCH 1/2] Minify error constructors in production We use a script to minify our error messages in production. Each message is assigned an error code, defined in `scripts/error-codes/codes.json`. Then our build script replaces the messages with a link to our error decoder page, e.g. https://reactjs.org/docs/error-decoder.html/?invariant=92 This enables us to write helpful error messages without increasing the bundle size. Right now, the script only works for `invariant` calls. It does not work if you throw an Error object. This is an old Facebookism that we don't really need, other than the fact that our error minification script relies on it. So, I've updated the script to minify error constructors, too: Input: Error(`A ${adj} message that contains ${noun}`); Output: Error(formatProdErrorMessage(ERR_CODE, adj, noun)); It only works for constructors that are literally named Error, though we could add support for other names, too. As a next step, I will add a lint rule to enforce that errors written this way must have a corresponding error code. --- packages/jest-react/src/JestReact.js | 12 +-- .../transform-error-messages.js.snap | 35 +++++++ .../__tests__/transform-error-messages.js | 57 +++++++++++ scripts/error-codes/codes.json | 2 - scripts/error-codes/extract-errors.js | 4 +- .../error-codes/transform-error-messages.js | 96 ++++++++++++++++++- scripts/print-warnings/print-warnings.js | 4 +- scripts/shared/__tests__/evalToString-test.js | 4 +- scripts/shared/evalToString.js | 40 +++++++- 9 files changed, 234 insertions(+), 20 deletions(-) diff --git a/packages/jest-react/src/JestReact.js b/packages/jest-react/src/JestReact.js index b4688f1ff980a..92f7e7cdb192d 100644 --- a/packages/jest-react/src/JestReact.js +++ b/packages/jest-react/src/JestReact.js @@ -7,7 +7,6 @@ import {REACT_ELEMENT_TYPE, REACT_FRAGMENT_TYPE} from 'shared/ReactSymbols'; -import invariant from 'shared/invariant'; import isArray from 'shared/isArray'; export {act} from './internalAct'; @@ -31,11 +30,12 @@ function captureAssertion(fn) { function assertYieldsWereCleared(root) { const Scheduler = root._Scheduler; const actualYields = Scheduler.unstable_clearYields(); - invariant( - actualYields.length === 0, - 'Log of yielded values is not empty. ' + - 'Call expect(ReactTestRenderer).unstable_toHaveYielded(...) first.', - ); + if (actualYields.length !== 0) { + throw new Error( + 'Log of yielded values is not empty. ' + + 'Call expect(ReactTestRenderer).unstable_toHaveYielded(...) first.', + ); + } } export function unstable_toMatchRenderedOutput(root, expectedJSX) { diff --git a/scripts/error-codes/__tests__/__snapshots__/transform-error-messages.js.snap b/scripts/error-codes/__tests__/__snapshots__/transform-error-messages.js.snap index f93b31a659dd2..bfb80ab375562 100644 --- a/scripts/error-codes/__tests__/__snapshots__/transform-error-messages.js.snap +++ b/scripts/error-codes/__tests__/__snapshots__/transform-error-messages.js.snap @@ -1,5 +1,10 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`error transform handles escaped backticks in template string 1`] = ` +"import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\"; +Error(__DEV__ ? \\"Expected \`\\" + listener + \\"\` listener to be a function, instead got a value of \`\\" + type + \\"\` type.\\" : _formatProdErrorMessage(231, listener, type));" +`; + exports[`error transform should correctly transform invariants that are not in the error codes map 1`] = ` "import invariant from 'shared/invariant'; @@ -18,6 +23,21 @@ if (!condition) { }" `; +exports[`error transform should not touch other calls or new expressions 1`] = ` +"new NotAnError(); +NotAnError();" +`; + +exports[`error transform should replace error constructors (no new) 1`] = ` +"import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\"; +Error(__DEV__ ? 'Do not override existing functions.' : _formatProdErrorMessage(16));" +`; + +exports[`error transform should replace error constructors 1`] = ` +"import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\"; +Error(__DEV__ ? 'Do not override existing functions.' : _formatProdErrorMessage(16));" +`; + exports[`error transform should replace simple invariant calls 1`] = ` "import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\"; import invariant from 'shared/invariant'; @@ -29,6 +49,21 @@ if (!condition) { }" `; +exports[`error transform should support error constructors with concatenated messages 1`] = ` +"import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\"; +Error(__DEV__ ? \\"Expected \\" + foo + \\" target to \\" + (\\"be an array; got \\" + bar) : _formatProdErrorMessage(7, foo, bar));" +`; + +exports[`error transform should support interpolating arguments with concatenation 1`] = ` +"import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\"; +Error(__DEV__ ? 'Expected ' + foo + ' target to be an array; got ' + bar : _formatProdErrorMessage(7, foo, bar));" +`; + +exports[`error transform should support interpolating arguments with template strings 1`] = ` +"import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\"; +Error(__DEV__ ? \\"Expected \\" + foo + \\" target to be an array; got \\" + bar : _formatProdErrorMessage(7, foo, bar));" +`; + exports[`error transform should support invariant calls with a concatenated template string and args 1`] = ` "import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\"; import invariant from 'shared/invariant'; diff --git a/scripts/error-codes/__tests__/transform-error-messages.js b/scripts/error-codes/__tests__/transform-error-messages.js index 4da00ac656b95..2bca7366321e2 100644 --- a/scripts/error-codes/__tests__/transform-error-messages.js +++ b/scripts/error-codes/__tests__/transform-error-messages.js @@ -93,4 +93,61 @@ invariant(condition, 'Do not override existing functions.'); ) ).toMatchSnapshot(); }); + + it('should replace error constructors', () => { + expect( + transform(` +new Error('Do not override existing functions.'); +`) + ).toMatchSnapshot(); + }); + + it('should replace error constructors (no new)', () => { + expect( + transform(` +Error('Do not override existing functions.'); +`) + ).toMatchSnapshot(); + }); + + it('should not touch other calls or new expressions', () => { + expect( + transform(` +new NotAnError(); +NotAnError(); +`) + ).toMatchSnapshot(); + }); + + it('should support interpolating arguments with template strings', () => { + expect( + transform(` +new Error(\`Expected \${foo} target to be an array; got \${bar}\`); +`) + ).toMatchSnapshot(); + }); + + it('should support interpolating arguments with concatenation', () => { + expect( + transform(` +new Error('Expected ' + foo + ' target to be an array; got ' + bar); +`) + ).toMatchSnapshot(); + }); + + it('should support error constructors with concatenated messages', () => { + expect( + transform(` +new Error(\`Expected \${foo} target to \` + \`be an array; got \${bar}\`); +`) + ).toMatchSnapshot(); + }); + + it('handles escaped backticks in template string', () => { + expect( + transform(` +new Error(\`Expected \\\`\$\{listener\}\\\` listener to be a function, instead got a value of \\\`\$\{type\}\\\` type.\`); +`) + ).toMatchSnapshot(); + }); }); diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index 1a6afe0233ed1..3c5f3d75a16eb 100644 --- a/scripts/error-codes/codes.json +++ b/scripts/error-codes/codes.json @@ -287,12 +287,10 @@ "288": "It is not supported to run the profiling version of a renderer (for example, `react-dom/profiling`) without also replacing the `schedule/tracing` module with `schedule/tracing-profiling`. Your bundler might have a setting for aliasing both modules. Learn more at https://reactjs.org/link/profiling", "289": "Function components cannot have refs.", "290": "Element ref was specified as a string (%s) but no owner was set. This could happen for one of the following reasons:\n1. You may be adding a ref to a function component\n2. You may be adding a ref to a component that was not created inside a component's render method\n3. You have multiple copies of React loaded\nSee https://reactjs.org/link/refs-must-have-owner for more information.", - "291": "Log of yielded values is not empty. Call expect(Scheduler).toHaveYielded(...) first.", "292": "The matcher `toHaveYielded` expects an instance of React Test Renderer.\n\nTry: expect(Scheduler).toHaveYielded(expectedYields)", "293": "Context can only be read while React is rendering, e.g. inside the render method or getDerivedStateFromProps.", "294": "ReactDOMServer does not yet support Suspense.", "295": "ReactDOMServer does not yet support lazy-loaded components.", - "296": "Log of yielded values is not empty. Call expect(ReactTestRenderer).unstable_toHaveYielded(...) first.", "297": "The matcher `unstable_toHaveYielded` expects an instance of React Test Renderer.\n\nTry: expect(ReactTestRenderer).unstable_toHaveYielded(expectedYields)", "298": "Hooks can only be called inside the body of a function component.", "299": "createRoot(...): Target container is not a DOM element.", diff --git a/scripts/error-codes/extract-errors.js b/scripts/error-codes/extract-errors.js index 58823cbe16873..d60ffe308cdbe 100644 --- a/scripts/error-codes/extract-errors.js +++ b/scripts/error-codes/extract-errors.js @@ -10,7 +10,7 @@ const parser = require('@babel/parser'); const fs = require('fs'); const path = require('path'); const traverse = require('@babel/traverse').default; -const evalToString = require('../shared/evalToString'); +const {evalStringConcat} = require('../shared/evalToString'); const invertObject = require('./invertObject'); const babylonOptions = { @@ -75,7 +75,7 @@ module.exports = function(opts) { // error messages can be concatenated (`+`) at runtime, so here's a // trivial partial evaluator that interprets the literal value - const errorMsgLiteral = evalToString(node.arguments[1]); + const errorMsgLiteral = evalStringConcat(node.arguments[1]); addToErrorMap(errorMsgLiteral); } }, diff --git a/scripts/error-codes/transform-error-messages.js b/scripts/error-codes/transform-error-messages.js index ee01454dbd886..2baf4baa1c1a7 100644 --- a/scripts/error-codes/transform-error-messages.js +++ b/scripts/error-codes/transform-error-messages.js @@ -7,7 +7,10 @@ 'use strict'; const fs = require('fs'); -const evalToString = require('../shared/evalToString'); +const { + evalStringConcat, + evalStringAndTemplateConcat, +} = require('../shared/evalToString'); const invertObject = require('./invertObject'); const helperModuleImports = require('@babel/helper-module-imports'); @@ -15,16 +18,103 @@ const errorMap = invertObject( JSON.parse(fs.readFileSync(__dirname + '/codes.json', 'utf-8')) ); +const SEEN_SYMBOL = Symbol('transform-error-messages.seen'); + module.exports = function(babel) { const t = babel.types; + // TODO: Instead of outputting __DEV__ conditions, only apply this transform + // in production. const DEV_EXPRESSION = t.identifier('__DEV__'); + function CallOrNewExpression(path, file) { + // Turns this code: + // + // new Error(`A ${adj} message that contains ${noun}`); + // + // or this code (no constructor): + // + // Error(`A ${adj} message that contains ${noun}`); + // + // into this: + // + // Error( + // __DEV__ + // ? `A ${adj} message that contains ${noun}` + // : formatProdErrorMessage(ERR_CODE, adj, noun) + // ); + const node = path.node; + if (node[SEEN_SYMBOL]) { + return; + } + node[SEEN_SYMBOL] = true; + + const errorMsgNode = node.arguments[0]; + if (errorMsgNode === undefined) { + return; + } + + const errorMsgExpressions = []; + const errorMsgLiteral = evalStringAndTemplateConcat( + errorMsgNode, + errorMsgExpressions + ); + + let prodErrorId = errorMap[errorMsgLiteral]; + if (prodErrorId === undefined) { + // There is no error code for this message. We use a lint rule to + // enforce that messages can be minified, so assume this is + // intentional and exit gracefully. + return; + } + prodErrorId = parseInt(prodErrorId, 10); + + // Import formatProdErrorMessage + const formatProdErrorMessageIdentifier = helperModuleImports.addDefault( + path, + 'shared/formatProdErrorMessage', + {nameHint: 'formatProdErrorMessage'} + ); + + // Outputs: + // formatProdErrorMessage(ERR_CODE, adj, noun); + const prodMessage = t.callExpression(formatProdErrorMessageIdentifier, [ + t.numericLiteral(prodErrorId), + ...errorMsgExpressions, + ]); + + // Outputs: + // Error( + // __DEV__ + // ? `A ${adj} message that contains ${noun}` + // : formatProdErrorMessage(ERR_CODE, adj, noun) + // ); + path.replaceWith(t.callExpression(t.identifier('Error'), [prodMessage])); + path.replaceWith( + t.callExpression(t.identifier('Error'), [ + t.conditionalExpression(DEV_EXPRESSION, errorMsgNode, prodMessage), + ]) + ); + } + return { visitor: { + NewExpression(path, file) { + const noMinify = file.opts.noMinify; + if (!noMinify && path.get('callee').isIdentifier({name: 'Error'})) { + CallOrNewExpression(path, file); + } + }, + CallExpression(path, file) { const node = path.node; const noMinify = file.opts.noMinify; + + if (!noMinify && path.get('callee').isIdentifier({name: 'Error'})) { + CallOrNewExpression(path, file); + return; + } + if (path.get('callee').isIdentifier({name: 'invariant'})) { // Turns this code: // @@ -44,7 +134,7 @@ module.exports = function(babel) { // string) that references a verbose error message. The mapping is // stored in `scripts/error-codes/codes.json`. const condition = node.arguments[0]; - const errorMsgLiteral = evalToString(node.arguments[1]); + const errorMsgLiteral = evalStringConcat(node.arguments[1]); const errorMsgExpressions = Array.from(node.arguments.slice(2)); const errorMsgQuasis = errorMsgLiteral .split('%s') @@ -115,7 +205,7 @@ module.exports = function(babel) { } prodErrorId = parseInt(prodErrorId, 10); - // Import ReactErrorProd + // Import formatProdErrorMessage const formatProdErrorMessageIdentifier = helperModuleImports.addDefault( path, 'shared/formatProdErrorMessage', diff --git a/scripts/print-warnings/print-warnings.js b/scripts/print-warnings/print-warnings.js index 9bdd6543d2a3d..8e23dd880e92b 100644 --- a/scripts/print-warnings/print-warnings.js +++ b/scripts/print-warnings/print-warnings.js @@ -12,7 +12,7 @@ const through = require('through2'); const traverse = require('@babel/traverse').default; const gs = require('glob-stream'); -const evalToString = require('../shared/evalToString'); +const {evalStringConcat} = require('../shared/evalToString'); const parserOptions = { sourceType: 'module', @@ -64,7 +64,7 @@ function transform(file, enc, cb) { // warning messages can be concatenated (`+`) at runtime, so here's // a trivial partial evaluator that interprets the literal value try { - const warningMsgLiteral = evalToString(node.arguments[0]); + const warningMsgLiteral = evalStringConcat(node.arguments[0]); warnings.add(JSON.stringify(warningMsgLiteral)); } catch (error) { console.error( diff --git a/scripts/shared/__tests__/evalToString-test.js b/scripts/shared/__tests__/evalToString-test.js index 3f6c3adbb372a..bc0cb600d57e4 100644 --- a/scripts/shared/__tests__/evalToString-test.js +++ b/scripts/shared/__tests__/evalToString-test.js @@ -6,12 +6,12 @@ */ 'use strict'; -const evalToString = require('../evalToString'); +const {evalStringConcat} = require('../evalToString'); const parser = require('@babel/parser'); const parse = source => parser.parse(`(${source});`).program.body[0].expression; // quick way to get an exp node -const parseAndEval = source => evalToString(parse(source)); +const parseAndEval = source => evalStringConcat(parse(source)); describe('evalToString', () => { it('should support StringLiteral', () => { diff --git a/scripts/shared/evalToString.js b/scripts/shared/evalToString.js index aad199da5d75b..318925cc0969e 100644 --- a/scripts/shared/evalToString.js +++ b/scripts/shared/evalToString.js @@ -8,7 +8,7 @@ */ 'use strict'; -function evalToString(ast /* : Object */) /* : string */ { +function evalStringConcat(ast /* : Object */) /* : string */ { switch (ast.type) { case 'StringLiteral': case 'Literal': // ESLint @@ -17,10 +17,44 @@ function evalToString(ast /* : Object */) /* : string */ { if (ast.operator !== '+') { throw new Error('Unsupported binary operator ' + ast.operator); } - return evalToString(ast.left) + evalToString(ast.right); + return evalStringConcat(ast.left) + evalStringConcat(ast.right); default: throw new Error('Unsupported type ' + ast.type); } } +exports.evalStringConcat = evalStringConcat; -module.exports = evalToString; +function evalStringAndTemplateConcat( + ast /* : Object */, + args /* : Array */ +) /* : string */ { + switch (ast.type) { + case 'StringLiteral': + return ast.value; + case 'BinaryExpression': // `+` + if (ast.operator !== '+') { + throw new Error('Unsupported binary operator ' + ast.operator); + } + return ( + evalStringAndTemplateConcat(ast.left, args) + + evalStringAndTemplateConcat(ast.right, args) + ); + case 'TemplateLiteral': { + let elements = []; + for (let i = 0; i < ast.quasis.length; i++) { + const elementNode = ast.quasis[i]; + if (elementNode.type !== 'TemplateElement') { + throw new Error('Unsupported type ' + ast.type); + } + elements.push(elementNode.value.cooked); + } + args.push(...ast.expressions); + return elements.join('%s'); + } + default: + // Anything that's not a string is interpreted as an argument. + args.push(ast); + return '%s'; + } +} +exports.evalStringAndTemplateConcat = evalStringAndTemplateConcat; From 7553658e78d4c2bab4992f1d2cc1207d3673e484 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sat, 25 Sep 2021 14:30:30 -0400 Subject: [PATCH 2/2] Minify "no fallback UI specified" error in prod MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This error message wasn't being minified because it doesn't use invariant. The reason it didn't use invariant is because this particular error is created without begin thrown — it doesn't need to be thrown because it's located inside the error handling part of the runtime. Now that the error minification script supports Error constructors, we can minify it by assigning it a production error code in `scripts/error-codes/codes.json`. To support the use of Error constructors more generally, I will add a lint rule that enforces each message has a corresponding error code. --- packages/react-reconciler/src/ReactFiberThrow.new.js | 3 ++- packages/react-reconciler/src/ReactFiberThrow.old.js | 3 ++- scripts/error-codes/codes.json | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberThrow.new.js b/packages/react-reconciler/src/ReactFiberThrow.new.js index 78e88f125a1aa..85728d96bb730 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.new.js +++ b/packages/react-reconciler/src/ReactFiberThrow.new.js @@ -474,7 +474,8 @@ function throwException( return; } else { // No boundary was found. Fallthrough to error mode. - // TODO: Use invariant so the message is stripped in prod? + // TODO: We should never call getComponentNameFromFiber in production. + // Log a warning or something to prevent us from accidentally bundling it. value = new Error( (getComponentNameFromFiber(sourceFiber) || 'A React component') + ' suspended while rendering, but no fallback UI was specified.\n' + diff --git a/packages/react-reconciler/src/ReactFiberThrow.old.js b/packages/react-reconciler/src/ReactFiberThrow.old.js index d2d39793a3bc0..27b0719ba8532 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.old.js +++ b/packages/react-reconciler/src/ReactFiberThrow.old.js @@ -474,7 +474,8 @@ function throwException( return; } else { // No boundary was found. Fallthrough to error mode. - // TODO: Use invariant so the message is stripped in prod? + // TODO: We should never call getComponentNameFromFiber in production. + // Log a warning or something to prevent us from accidentally bundling it. value = new Error( (getComponentNameFromFiber(sourceFiber) || 'A React component') + ' suspended while rendering, but no fallback UI was specified.\n' + diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index 3c5f3d75a16eb..d11735986235a 100644 --- a/scripts/error-codes/codes.json +++ b/scripts/error-codes/codes.json @@ -394,5 +394,6 @@ "405": "hydrateRoot(...): Target container is not a DOM element.", "406": "act(...) is not supported in production builds of React.", "407": "Missing getServerSnapshot, which is required for server-rendered content. Will revert to client rendering.", - "408": "Missing getServerSnapshot, which is required for server-rendered content." + "408": "Missing getServerSnapshot, which is required for server-rendered content.", + "409": "%s suspended while rendering, but no fallback UI was specified.\n\nAdd a component higher in the tree to provide a loading indicator or placeholder to display." }