Skip to content

Update error minification script to support Error call expressions #22428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions packages/jest-react/src/JestReact.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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.',
);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed this was being minified in jest-react but that's an internal module so there's no reason to. The extract-errors script must have picked this up at some point. Since it's not part of a public bundle we don't have to minify it.

}

export function unstable_toMatchRenderedOutput(root, expectedJSX) {
Expand Down
3 changes: 2 additions & 1 deletion packages/react-reconciler/src/ReactFiberThrow.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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' +
Expand Down
3 changes: 2 additions & 1 deletion packages/react-reconciler/src/ReactFiberThrow.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -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' +
Expand Down
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -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';
Expand All @@ -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';
Expand Down
57 changes: 57 additions & 0 deletions scripts/error-codes/__tests__/transform-error-messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
5 changes: 2 additions & 3 deletions scripts/error-codes/codes.json
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Copy link
Collaborator Author

@acdlite acdlite Sep 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://github.com/facebook/react/pull/22428/files#r716105083. Since this message was never part of a public bundle we can remove it.

"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.",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

"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.",
Expand Down Expand Up @@ -397,5 +395,6 @@
"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.",
"409": "Cannot update an unmounted root."
"409": "Cannot update an unmounted root.",
"410": "%s suspended while rendering, but no fallback UI was specified.\n\nAdd a <Suspense fallback=...> component higher in the tree to provide a loading indicator or placeholder to display."
}
4 changes: 2 additions & 2 deletions scripts/error-codes/extract-errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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);
}
},
Expand Down
106 changes: 97 additions & 9 deletions scripts/error-codes/transform-error-messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,114 @@
'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');

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:
//
Expand All @@ -40,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')
Expand Down Expand Up @@ -81,12 +175,6 @@ module.exports = function(babel) {
return;
}

// Avoid caching because we write it as we go.
const existingErrorMap = JSON.parse(
fs.readFileSync(__dirname + '/codes.json', 'utf-8')
);
const errorMap = invertObject(existingErrorMap);

let prodErrorId = errorMap[errorMsgLiteral];

if (prodErrorId === undefined) {
Expand Down Expand Up @@ -117,7 +205,7 @@ module.exports = function(babel) {
}
prodErrorId = parseInt(prodErrorId, 10);

// Import ReactErrorProd
// Import formatProdErrorMessage
const formatProdErrorMessageIdentifier = helperModuleImports.addDefault(
path,
'shared/formatProdErrorMessage',
Expand Down
4 changes: 2 additions & 2 deletions scripts/print-warnings/print-warnings.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions scripts/shared/__tests__/evalToString-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
Loading