refactor(transformer):
8000
don't need to transform JSXExpression::EmptyExpression in JSX attributes
#8817
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
JSXExpression::EmptyExpression in JSX attributes
90a2999 to
5aea3cf
Compare
c7edd4f to
b1d0bb7
Compare
CodSpeed Performance ReportMerging #8817 will not alter performanceComparing Summary
|
| JSXExpression::EmptyExpression(_) => { | ||
| // `<div attr={}></div>` | ||
| // ^^ Invalid empty expression here | ||
| unreachable!() |
There was a problem hiding this comment.
We should not make this unreachable in situations where the semantic checker is not run or missed the check, do not emit anything instead.
There was a problem hiding this comment.
That makes sense. To achieve "do not emit anything" here we must return a None, which will cause more changes, I will do this later.
There was a problem hiding this comment.
From reading the JSX spec, it doesn't look like an EmptyExpression is ever valid in this position. If I've understood that right, could we make JSXExpressionContainer just contain an Expression, rather than a JSXExpression?
I assume the reason is recoverability. To achieve that, could we instead raise an error in the parser, and skip adding the JSXAttribute, so that parsing can continue?
It's going to cause a problem serializing to ESTree, because what does it serialize to? The "EmptyExpression" option doesn't exist.
There was a problem hiding this comment.
This is part of typescript-eslint, See https://ast.sxzz.dev/#eNpNzMEKwjAMgOFXGTlPvI+5o08gnnoJNUhH15SmFaX03Y1WZLfk+0kqeJhgxQeKTS5mGCEq5FekDgcS78LHrfp8Zh7wVNtiwlCbCfNRZdHKWqviYEC4JEsXfWFg0n3jW/E6jz2T3fBKSRyH3j1mkvzvCcP9e5lToZ+t8txJg/YGroc72A==.
I agree to throw an error in the parser as this is a pointless AST node.
b1d0bb7 to
cba6a7a
Compare
5aea3cf to
b00b8c8
Compare
…pression in JSX attributes
cba6a7a to
1c1add7
Compare
|
Re changing AST: I double checked, it's a difficult change because we want the span in Close as not planned. |
No description provided.