TypeScript: format TSAsExpression with same logic as BinaryExpression

[kaicataldo]

fixes #6443.

I took a stab at working on wrapping TSAsExpression nodes! In taking a look at this, it looks like the behavior of moving the initial declaration value up to the same line as the = operator happens with other declarations and I think the change might have to happen in that logic (here's an example using the Prettier playground).

This PR instead focuses on wrapping the TSAsExpression node correctly (ensuring that the line break occurs after the as operator).

This now takes a different approach and converts the TSAsExpression node to match the BinaryExpression node interface so we can use the same formatting logic for both.

I'm pretty new to the Prettier codebase, and any and all guidance is very welcome!

• I’ve added tests to confirm my change works.
• (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
• (If the change is user-facing) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
• I’ve read the contributing guidelines.

✨Try the playground for this PR✨

[alexander-akait]

Need better check, regexp is bad in this case

[kaicataldo]

After thinking about this some more, I took a completely different approach. It feels like we should treat as operators similarly to any other binary operator. By postprocessing the TSAsExpression to match the interface of a BinaryExpression, we can use the exact same logic. Thoughts?

[kaicataldo]

I'm having trouble recreating these test failures locally. Any idea what might be going on? I'm running them on Node v10.16.3 in macOS v10.14.6.

[lydell]

Feels pretty hacky to me, but I'm not sure how to do it better. There's quite a bit of code for printing binary expressions. On the other hand – do as expressions really need to go through all of the binary expression stuff, like the JSX related parts? Maybe there’s a way to extract or copy the parts needed for as?

@vjeux You have had useful insight before on PRs with hacky code – what does your experience tell you here (gut feeling)?

[kaicataldo]

Maybe there’s a way to extract or copy the parts needed for as?

My thought here was that keeping the two in sync would be a bigger maintenance burden, but if that's the preferred way of doing things, I can definitely look into it!

I'm also coming from working on babel-eslint, where the parser has to do a lot of massaging of the AST to get it to work with ESLint. 😅

[thorn0]

Does it modify the passed AST itself or a copy of it? What if someone else wanted to use that AST after Prettier? Or is such a scenario already unsupported anyway?

[thorn0]

do as expressions really need to go through all of the binary expression stuff, like the JSX related parts

Why not? as can be used anywhere, just like + or say &&,

[kaicataldo]

Happy to work on this again once the team decides what the preferred approach is!

[thorn0]

Another case of aligning the formatting of as with how other binary expressions are formatted: #6471

[thorn0]

I'm having trouble recreating these test failures locally. Any idea what might be going on? I'm running them on Node v10.16.3 in macOS v10.14.6.

Do you run the tests with AST_COMPARE=1?

[kaicataldo]

@thorn0 Ah, thanks! That's what I was missing!

[kaicataldo]

Friendly ping for the Prettier team - any insight on how we'd like to proceed here?

[alexander-akait]

@kaicataldo need fix CI problems

[thorn0]

I don't represent the Prettier team, but I expect them to support the following points:

1. formatting as-expressions like binary expressions makes total sense, but
2. modifying the AST is not okay (e.g. see this case).

[kaicataldo]

@thorn0 @lydell Thanks, that should unblock me. I'll get to this ASAP!

[kaicataldo]

I'm revisiting this tonight, and it looks like our options are either to:

1. Clone the node and modify it to match the BinaryExpression interface before printing.
2. Copy over the current printing BinaryExpression logic and modify it to work with the current TSAsExpression node interface.

The challenge of the first strategy is that we have to convert the TSAsExpression node every time it might exist (which is lots of places! e.g. here), which adds a lot of complexity throughout the printer.

The challenge of the latter is that we'll have a fork in that logic and will have to make future changes in both code paths. This feels more feasible, but still doesn't feel great to me.

Thoughts?

[thorn0]

Might it make sense to replace all occurrences of .operator, .left and .right with function calls (getOperator, etc)?

[duailibe]

@kaicataldo I think we should start with option 2 and then work from that to minimize the problems with forking later

[thorn0]

On the playground for this PR, I see that as is not formatted like other binaries (e.g. + or **), at least in this specific case (from #6623):

Prettier pr-6450
Playground link

--parser typescript

Input:

const iter1 = createIterator(this.controller, child, this.tag as SyncFunctionComponent);
const iter2 = createIterator(self.controller, child, self.tag as SyncFunctionComponent);

const iter1 = createIterator(this.controller, child, this.tag ** SyncFunctionComponent);
const iter2 = createIterator(self.controller, child, self.tag ** SyncFunctionComponent);

Output:

const iter1 = createIterator(this.controller, child, this.tag as
  SyncFunctionComponent);
const iter2 = createIterator(self.controller, child, self.tag as
  SyncFunctionComponent);

const iter1 = createIterator(
  this.controller,
  child,
  this.tag ** SyncFunctionComponent
);
const iter2 = createIterator(
  self.controller,
  child,
  self.tag ** SyncFunctionComponent
);

[kaicataldo]

Sorry, haven’t been able to get back to this. Will close this for now so that someone else can work on it.