Skip to content

Update error transform to allow excluding errors inside subexpressions like ternaries #24693

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.

Sign up for GitHub

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

Merged
gnoff merged 2 commits into from
Jun 8, 2022
+86 −1
Merged

Update error transform to allow excluding errors inside subexpressions like ternaries #24693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d381162
Update error transform to allow excluding errors inside subexpression…
gnoff Jun 8, 2022
9ab80c5
make leadingcomments aggregation walk the expression stack
gnoff Jun 8, 2022
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
24 changes: 24 additions & 0 deletions scripts/error-codes/__tests__/__snapshots__/transform-error-messages.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,34 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`error transform handles deeply nested expressions 1`] = `
"var val = (a, (b, // eslint-disable-next-line react-internal/prod-error-codes
new Error('foo')));"
`;

exports[`error transform handles deeply nested expressions 2`] = `
"var val = (a, ( // eslint-disable-next-line react-internal/prod-error-codes
b, new Error('foo')));"
`;

exports[`error transform handles escaped backticks in template string 1`] = `
"import _formatProdErrorMessage from \\"shared/formatProdErrorMessage\\";
Error(_formatProdErrorMessage(231, listener, type));"
`;

exports[`error transform handles ignoring errors that are comment-excluded inside ternary expressions 1`] = `
"/*! FIXME (minify-errors-in-prod): Unminified error message in production build!*/

/*! <expected-error-format>\\"bar\\"</expected-error-format>*/
var val = someBool ? //eslint-disable-next-line react-internal/prod-error-codes
new Error('foo') : someOtherBool ? new Error('bar') : //eslint-disable-next-line react-internal/prod-error-codes
new Error('baz');"
`;

exports[`error transform handles ignoring errors that are comment-excluded outside ternary expressions 1`] = `
"//eslint-disable-next-line react-internal/prod-error-codes
var val = someBool ? new Error('foo') : someOtherBool ? new Error('bar') : new Error('baz');"
`;

exports[`error transform should not touch other calls or new expressions 1`] = `
"new NotAnError();
NotAnError();"
Expand Down
48 changes: 48 additions & 0 deletions scripts/error-codes/__tests__/transform-error-messages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,54 @@ new Error(\`Expected \${foo} target to \` + \`be an array; got \${bar}\`);
expect(
transform(`
new Error(\`Expected \\\`\$\{listener\}\\\` listener to be a function, instead got a value of \\\`\$\{type\}\\\` type.\`);
`)
).toMatchSnapshot();
});

it('handles ignoring errors that are comment-excluded inside ternary expressions', () => {
expect(
transform(`
let val = someBool
? //eslint-disable-next-line react-internal/prod-error-codes
new Error('foo')
: someOtherBool
? new Error('bar')
: //eslint-disable-next-line react-internal/prod-error-codes
new Error('baz');
`)
).toMatchSnapshot();
});

it('handles ignoring errors that are comment-excluded outside ternary expressions', () => {
expect(
transform(`
//eslint-disable-next-line react-internal/prod-error-codes
let val = someBool
? new Error('foo')
: someOtherBool
? new Error('bar')
: new Error('baz');
`)
).toMatchSnapshot();
});

it('handles deeply nested expressions', () => {
expect(
transform(`
let val =
(a,
(b,
// eslint-disable-next-line react-internal/prod-error-codes
new Error('foo')));
`)
).toMatchSnapshot();

expect(
transform(`
let val =
(a,
// eslint-disable-next-line react-internal/prod-error-codes
(b, new Error('foo')));
`)
).toMatchSnapshot();
});
Expand Down
15 changes: 14 additions & 1 deletion scripts/error-codes/transform-error-messages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,21 @@ module.exports = function(babel) {
// throw Error(`A ${adj} message that contains ${noun}`);
// }

let leadingComments = [];

const statementParent = path.getStatementParent();
const leadingComments = statementParent.node.leadingComments;
let nextPath = path;
while (true) {
let nextNode = nextPath.node;
if (nextNode.leadingComments) {
leadingComments.push(...nextNode.leadingComments);
}
if (nextPath === statementParent) {
break;
}
nextPath = nextPath.parentPath;
}
Copy link
Collaborator

@acdlite acdlite Jun 8, 2022

Choose a reason for hiding this comment

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

What if there are multiple nested expressions? Like nested ternaries. Maybe it should check all the nodes until it hits the statement parent.

Copy link
Collaborator Author

@gnoff gnoff Jun 8, 2022

Choose a reason for hiding this comment

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

I like that idea but the lint functionality doesn't follow expression semantics, it seems to actually follow line semantics. If we did this, it would be possible for the linter to take exception to an error that the transformer would ignore. maybe that's ok? but extract errors wouldn't work to auto-fix any errors the linter had a problem with

without changing anything we have

// linter ignores error, transformer flags error ❌
let val =
  // eslint-disable-next-line react-internal/prod-error-codes
  (b, new Error('foo'));

// linter flags error, transformer flags error ✅
let val =
  // eslint-disable-next-line react-internal/prod-error-codes
  (b,
  // Some random comment
  new Error('foo'));

if we walk up expressions we have

// linter ignores error, transformer ignores error ✅
let val =
  // eslint-disable-next-line react-internal/prod-error-codes
  (b, new Error('foo'));

// linter flags error, transformer ignores error ❌
let val =
  // eslint-disable-next-line react-internal/prod-error-codes
  (b,
  // Some random comment
  new Error('foo'));

Copy link
Collaborator Author

@gnoff gnoff Jun 8, 2022

Choose a reason for hiding this comment

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

perhaps the latter one is actually better? it would make the linter more aggressive than the transformer. Was the point of the transformer to be a more strict guard?

Copy link
Collaborator

@acdlite acdlite Jun 8, 2022

Choose a reason for hiding this comment

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

Was the point of the transformer to be a more strict guard?

Conceptually it's just another lint rule, except it runs on the build artifacts instead of the source files. It's an additional failsafe in case ESLint was misconfigured, like if you pull in a function from another module and the source files for that module weren't checked by ESLint.

Neither the FIXME comments nor the ESLint rule affect whether the messages get minified — they will cause CI to fail, but they won't change anything about the AST (aside from the additional comments).

The only thing that determines whether a message minified is its presence in codes.json. So the purpose of both the transformer and the lint rule is to remind you to update that file.

Copy link
Collaborator Author

@gnoff gnoff Jun 8, 2022

Choose a reason for hiding this comment

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

I guess I was concerned that FIXME's in the build output were bad b/c they would leak. but practically speaking they are probably always bundled so the extra bytes are maybe getting stripped out anyway.

Either way, I agree it's nice to be able to put the opt-out in a more flexible location and updated the implementation to walk up to the statement parent. lmk what you think

Copy link
Collaborator

@acdlite acdlite Jun 8, 2022

Choose a reason for hiding this comment

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

I guess I was concerned that FIXME's in the build output were bad b/c they would leak. but practically speaking they are probably always bundled so the extra bytes are maybe getting stripped out anyway.

Well I don't mean to say it's fine if those land in the build. That's why the CI job fails if it detects any. The point is just that the author needs to make an intentional decision either way. So as long as either the check_error_codes job or the lint rule catches it, it's fine. It's just even better if the lint rule covers all cases because it provides the nicer dev experience, since you don't have to build first.

Copy link
Collaborator Author

@gnoff gnoff Jun 8, 2022

Choose a reason for hiding this comment

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

yeah makes sense. the error_codes job in CI working is an invariant I suppose we should be able to to expect

Copy link

@Nadineersej Nadineersej Oct 31, 2023

Choose a reason for hiding this comment

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

Yes


if (leadingComments !== undefined) {
for (let i = 0; i < leadingComments.length; i++) {
// TODO: Since this only detects one of many ways to disable a lint
Expand Down