Update error transform to allow excluding errors inside subexpressions like ternaries #24693
Conversation
|
@acdlite lmk if you think this change makes sense? Also curious to see if it affects the bundles. It shouldn't but if it does I'll know something else is going on than what I expect this code to do |
|
Comparing: 42b330c...9ab80c5 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
| leadingComments = leadingComments | ||
| ? leadingComments.concat(parentLeadingComments) | ||
| : parentLeadingComments; | ||
| } |
There was a problem hiding this comment.
What if there are multiple nested expressions? Like nested ternaries. Maybe it should check all the nodes until it hits the statement parent.
There was a problem hiding this comment.
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'));
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yeah makes sense. the error_codes job in CI working is an invariant I suppose we should be able to to expect
in #24688 the we refactored a ternary assignment to conditional assignment because the transform-error-messages babel plugin was not respecting the minified error opt-out embedded in the ternary.
This PR updates the transform-error-messages plugin to consider the Error expression's leading comments in addition to the Error expression's parent leading comments.