From 1d592e810f217fdb01ba30a2e365806d3a2c17d0 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 6 Oct 2021 09:06:30 -0400 Subject: [PATCH] Output FIXME during build for unminified errors The invariant Babel transform used to output a FIXME comment if it could not find a matching error code. This could happen if there were a configuration mistake that caused an unminified message to slip through. Linting the compiled bundles is the most reliable way to do it because there's not a one-to-one mapping between source modules and bundles. For example, the same source module may appear in multiple bundles, some which are minified and others which aren't. This updates the transform to output the same messages for Error calls. The source lint rule is still useful for catching mistakes during development, to prompt you to update the error codes map before pushing the PR to CI. --- .../transform-error-messages.js.snap | 10 +++ .../__tests__/transform-error-messages.js | 25 ++++++ .../error-codes/transform-error-messages.js | 84 ++++++++++++------- 3 files changed, 89 insertions(+), 30 deletions(-) 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 bfb80ab375562..034e7ff5e903e 100644 --- a/scripts/error-codes/__tests__/__snapshots__/transform-error-messages.js.snap +++ b/scripts/error-codes/__tests__/__snapshots__/transform-error-messages.js.snap @@ -28,6 +28,16 @@ exports[`error transform should not touch other calls or new expressions 1`] = ` NotAnError();" `; +exports[`error transform should output FIXME for errors that don't have a matching error code 1`] = ` +"/*FIXME (minify-errors-in-prod): Unminified error message in production build!*/ +Error(condition, 'This is not a real error message.');" +`; + +exports[`error transform should output FIXME for errors that don't have a matching error code, unless opted out with a comment 1`] = ` +"// eslint-disable-next-line react-internal/prod-error-codes +Error(condition, 'This is not a real error message.');" +`; + exports[`error transform should replace error constructors (no new) 1`] = ` "import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\"; Error(__DEV__ ? 'Do not override existing functions.' : _formatProdErrorMessage(16));" diff --git a/scripts/error-codes/__tests__/transform-error-messages.js b/scripts/error-codes/__tests__/transform-error-messages.js index 2bca7366321e2..a15515a050f9e 100644 --- a/scripts/error-codes/__tests__/transform-error-messages.js +++ b/scripts/error-codes/__tests__/transform-error-messages.js @@ -110,6 +110,31 @@ Error('Do not override existing functions.'); ).toMatchSnapshot(); }); + it("should output FIXME for errors that don't have a matching error code", () => { + expect( + transform(` +Error(condition, 'This is not a real error message.'); +`) + ).toMatchSnapshot(); + }); + + it( + "should output FIXME for errors that don't have a matching error " + + 'code, unless opted out with a comment', + () => { + // TODO: Since this only detects one of many ways to disable a lint + // rule, we should instead search for a custom directive (like + // no-minify-errors) instead of ESLint. Will need to update our lint + // rule to recognize the same directive. + expect( + transform(` +// eslint-disable-next-line react-internal/prod-error-codes +Error(condition, 'This is not a real error message.'); +`) + ).toMatchSnapshot(); + } + ); + it('should not touch other calls or new expressions', () => { expect( transform(` diff --git a/scripts/error-codes/transform-error-messages.js b/scripts/error-codes/transform-error-messages.js index 2baf4baa1c1a7..7e90835279959 100644 --- a/scripts/error-codes/transform-error-messages.js +++ b/scripts/error-codes/transform-error-messages.js @@ -62,9 +62,38 @@ module.exports = function(babel) { 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. + // There is no error code for this message. Add an inline comment + // that flags this as an unminified error. This allows the build + // to proceed, while also allowing a post-build linter to detect it. + // + // Outputs: + // /* FIXME (minify-errors-in-prod): Unminified error message in production build! */ + // if (!condition) { + // throw Error(`A ${adj} message that contains ${noun}`); + // } + + const leadingComments = path.parentPath.node.leadingComments; + if (leadingComments !== undefined) { + for (let i = 0; i < leadingComments.length; i++) { + // TODO: Since this only detects one of many ways to disable a lint + // rule, we should instead search for a custom directive (like + // no-minify-errors) instead of ESLint. Will need to update our lint + // rule to recognize the same directive. + const commentText = leadingComments[i].value; + if ( + commentText.includes( + 'eslint-disable-next-line react-internal/prod-error-codes' + ) + ) { + return; + } + } + } + + path.parentPath.addComment( + 'leading', + 'FIXME (minify-errors-in-prod): Unminified error message in production build!' + ); return; } prodErrorId = parseInt(prodErrorId, 10); @@ -89,12 +118,11 @@ module.exports = function(babel) { // ? `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), - ]) - ); + const newErrorCall = t.callExpression(t.identifier('Error'), [ + t.conditionalExpression(DEV_EXPRESSION, errorMsgNode, prodMessage), + ]); + newErrorCall[SEEN_SYMBOL] = true; + path.replaceWith(newErrorCall); } return { @@ -162,16 +190,17 @@ module.exports = function(babel) { // if (!condition) { // throw Error(`A ${adj} message that contains ${noun}`); // } + const errorCallNode = t.callExpression(t.identifier('Error'), [ + devMessage, + ]); + errorCallNode[SEEN_SYMBOL] = true; parentStatementPath.replaceWith( t.ifStatement( t.unaryExpression('!', condition), - t.blockStatement([ - t.throwStatement( - t.callExpression(t.identifier('Error'), [devMessage]) - ), - ]) + t.blockStatement([t.throwStatement(errorCallNode)]) ) ); + return; } @@ -187,14 +216,14 @@ module.exports = function(babel) { // if (!condition) { // throw Error(`A ${adj} message that contains ${noun}`); // } + const errorCall = t.callExpression(t.identifier('Error'), [ + devMessage, + ]); + errorCall[SEEN_SYMBOL] = true; parentStatementPath.replaceWith( t.ifStatement( t.unaryExpression('!', condition), - t.blockStatement([ - t.throwStatement( - t.callExpression(t.identifier('Error'), [devMessage]) - ), - ]) + t.blockStatement([t.throwStatement(errorCall)]) ) ); parentStatementPath.addComment( @@ -219,6 +248,11 @@ module.exports = function(babel) { [t.numericLiteral(prodErrorId), ...errorMsgExpressions] ); + const errorCall = t.callExpression(t.identifier('Error'), [ + t.conditionalExpression(DEV_EXPRESSION, devMessage, prodMessage), + ]); + errorCall[SEEN_SYMBOL] = true; + // Outputs: // if (!condition) { // throw Error( @@ -231,17 +265,7 @@ module.exports = function(babel) { t.ifStatement( t.unaryExpression('!', condition), t.blockStatement([ - t.blockStatement([ - t.throwStatement( - t.callExpression(t.identifier('Error'), [ - t.conditionalExpression( - DEV_EXPRESSION, - devMessage, - prodMessage - ), - ]) - ), - ]), + t.blockStatement([t.throwStatement(errorCall)]), ]) ) );