Ultimate fix for Closure-style type casts #7791
Conversation
| const v2 = /** @type {}} */ (value); | ||
| const v3 = /** @type } */ (value); | ||
| const v4 = /** @type { */ (value); | ||
| const v5 = /** @type {{} */ (value); |
There was a problem hiding this comment.
We should ignore it and keep as is
There was a problem hiding this comment.
I don't think we should. We might try checking the syntax of types in these simple cases, but what about more complex ones? Those types can contain everything, e.g. curly braces inside quotes.
There was a problem hiding this comment.
That's what I mean:
const curly = /** @type { '}' | '{' | '}}' } */ (getCurly());There was a problem hiding this comment.
Also, as I wrote in comments in the code:
TypeScript expects the type to be enclosed in curly brackets,
however Closure Compiler accepts types in parens and even without any delimiters at all.
That's why we only check that there is at least one non-whitespace character after@type.
There was a problem hiding this comment.
Sound reasonable, but on the other hand we can add unnecessary parens when somebody has @type in comments, yes, it is edge case, let's keep it as is, if somebody open a issue about it we will improve it
There was a problem hiding this comment.
This is a very edge case. One has to have both @type in a comment AND a parenthesized expression after that comment.
alexander-akait
left a comment
alexander-akait
left a comment
There was a problem hiding this comment.
Looks very very very good, can we list of fixes issues?
thorn0
force-pushed
the
closure-type-casts
branch
from
43ad2a9 to
6b12525
Compare
src/language-js/comments.js
Outdated
| } | ||
| } | ||
|
|
||
| function removeJsDocAsterisks(commentValue) { |
There was a problem hiding this comment.
I can't find a test case that actually requires this. I think the intention is to exclude leading * after a newline after the @type annotation? If so, I think it'd be plenty fine to just search for @type and not care about having a non-whitespace char.
There was a problem hiding this comment.
Agree. This is leftover code from the check for curly braces around the type.
| } | ||
| }); | ||
|
|
||
| visitNode(ast, node => { |
There was a problem hiding this comment.
We could probably do this in a single walk, since the ancestor will have been visited first.
| const { expression } = node; | ||
| if (!expression.extra) { | ||
| expression.extra = {}; | ||
| } | ||
| expression.extra.parenthesized = true; | ||
| expression.extra.parenStart = node.start; | ||
| return expression; |
There was a problem hiding this comment.
What about open a issue in babel repo about it? I think they can improve, we have pretty good reasons for this
There was a problem hiding this comment.
I don't see what you mean. Creating this extra stuff is Babel's default behavior, but it's not created when createParenthesizedExpressions is on. However, some other code in Prettier relied on extra.parenthesized , so I couldn't just enable createParenthesizedExpressions without recreating extra.
thorn0
force-pushed
the
closure-type-casts
branch
from
32b74d2 to
875dc1d
Compare
5 participants
Supersedes #7782
Fixes #4124
Fixes #6122
Fixes #7775
Fixes #7686
docs/directory)changelog_unreleased/*/pr-XXXX.mdfile followingchangelog_unreleased/TEMPLATE.md.✨Try the playground for this PR✨