-
-
Notifications
You must be signed in to change notification settings - Fork 511
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
challenge(formatter): edge case with expression statement and type cast #754
challenge(formatter): edge case with expression statement and type cast #754
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix looks good to me. Although could you try something? Try to commit the suggested fix with JsIdentifierExpression::needs_parentheses_with_parent
, and bench the result. And then bench this solution. If there aren't big differences, we could use the needs_parentheses_with_parent
solution, which is more idiomatic against our architecture.
Remember to add a comment to #739 once this is merged
!bench_formatter |
Formatter Benchmark Results
|
c08ea3f
to
bb5f62b
Compare
Good news: we no longer diverge from prettier. This was addressed in prettier#15514. |
!bench_formatter |
bb5f62b
to
c1626f5
Compare
Formatter Benchmark Results
|
!bench_formatter |
Formatter Benchmark Results
|
f7a7693
to
26b67cb
Compare
Summary
This fixes the parse error for
typescript/as/expression-statement.ts
.Unfortunately we have to diverge from prettier here. Thus, this does not improve the compatibility score. However, this should not make it worse.
The following code is valid in TypeScript:
Prettier and Biome currently format this code into an invalid code:
I added an edge case check in the formatting code of
as
expressions.It adds parens when the parent node is a statement and the expression is the identifier
type
.Alternatively, this logic could be implemented in
JsIdnetifierExpression::needs_parentheses
. However, tis could incur a perf regression because identifier expression are more common thanas
expressions.Test Plan
Updated tests.