Skip to content

Commit

Permalink
Output FIXME during build for unminified errors
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite committed Oct 6, 2021
1 parent 6ecad79 commit a81d95c
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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));"
Expand Down
25 changes: 25 additions & 0 deletions scripts/error-codes/__tests__/transform-error-messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(`
Expand Down
84 changes: 54 additions & 30 deletions scripts/error-codes/transform-error-messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.addComment(
'leading',
'FIXME (minify-errors-in-prod): Unminified error message in production build!'
);
return;
}
prodErrorId = parseInt(prodErrorId, 10);
Expand All @@ -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 {
Expand Down Expand Up @@ -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;
}

Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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)]),
])
)
);
Expand Down

0 comments on commit a81d95c

Please sign in to comment.