Avoid extra offset in arrow function body when using long types

[kachkaev]

Description

Closes #10848

Checklist

• 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 changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
• I’ve read the contributing guidelines.

✨Try the playground for this PR✨

[kachkaev]

I think I’ve understood the general idea behind what should be done, but I’m definitely not there yet 😅

The new tests are doing reasonably well – all snapshots pass except this one:

- // Expected
- const MyComponent2: (
-   React.VoidFunctionComponent<MyComponent2Props>
- ) = ({ x, y }) => {
+ // Received
+ const MyComponent2: React.VoidFunctionComponent<MyComponent2Props> = ({
+   x,
+   y,
+ }) => {

I guess it’s because the arguments are also a group. Prettier manages to fit the first group, then breaks the second one. Not sure how to change the priorities here.

The bad news is that a lot of existing snapshots have failed because of my change 😁 We now have (\n type\n) in too many places. So if I understand correctly, instead of making the change around case "TSTypeAnnotation":, we need something like:

    case "break-lhs":
-     return group([leftDoc, operator, " ", group(rightDoc)]);
+     return group([tweak(leftDoc), operator, " ", group(rightDoc)]);

Not sure if this is ‘legal’ in Prettier 🤔 Maybe there is another way that I haven’t discovered yet?

[kachkaev]

UPD: I reverted src/language-js/print/typescript.js and introduced makeTypeAnnotationBreakable as POC for tweak(leftDoc). Happy to use a completely different approach if ‘hacking’ a doc is not what we should be doing.

I haven’t updated the snapshots yet to emphasise the problem with MyComponent2 as described above. There are more failing snapshots and I’m happy to update them if the delta makes sense.

[thorn0]

Hacking a doc is indeed a last resort kind of thing. Introspecting a doc, on the other hand, is usually a better idea. In the place where the type annotation is printed, you can wrap the resulting doc with breaks and parens if it can't break. To check that it can't break, you can use this function (add it to document/doc-utils.js):

function canBreakFn(doc) {
  if (doc.type === "line") {
    return true;
  }
}
function canBreak(doc) {
  return findInDoc(doc, canBreakFn, false);
}

[kachkaev]

Thanks for the suggestion @thorn0! I tried implementing canBreak – hope that I understood you more or less correctly. We now add wrapping in case "TSTypeAnnotation". Two observations:

• The snapshot for MyComponent2 is still including ({\nx,\ny,\n}) instead of (\nReact.VoidFUnctionComponent<...>\n), which is not ideal.
• Formatting seems to be unstable now because I can’t get all tests pass by calling yarn test -u.

I updated the snapshots for both old an new solution to help us see where we are. To be honest, what we’ve had with makeTypeAnnotationBreakable looks a bit more attractive to me from the output POV. That solution only adds wrapping when going for break-lhs. I wonder if we can somehow achieve that but without having to ‘hack’ docs. WDYT?

[kachkaev]

@thorn0 🙏

[kachkaev]

@thorn0 I merged the latest main branch into my branch and resolved minor conflicts. The solutions are now swapped.

main...ca62753 contains a non-hacky solutions which I could not complete
main...eb1150a (latest) contains makeTypeAnnotationBreakable, which is a bit hacky but works.

Happy to make further steps in any of the two directions 🙂

[kachkaev]

👋 @thorn0 / @sosukesuzuki

Given that 2.3.1 can be released quite soon, what do you think of reverting this PR to 89e92bd (with ✅) and merging?

The implementation I have there is not ideal because it is based on tweaking the intermediate representation (doc). However, it fully solves an issue that prevents the migration from 2.2 to 2.3 for some folks.

If we manage to include a technically imperfect solution into 2.3.1, I’m happy to work on the follow-up PR with your help and replace the hack with the intermediate representation. See details in this discussion: #10956 (review).

[sosukesuzuki]

@kachkaev Thanks for the suggestion. Sorry, but I think I'll skip this it for 2.3.2. We can patch-release it in multiple times, so let's release this PR as 2.3.2.

[jkillian]

I love the idea of preventing extra indentation and appreciate a PR to improve that, but I do wonder about this specific to solve it. Parenthesizing the type annotation feels a bit unusual/non-idiomatic? Are there other situations right now where types get parenthesized similarly?

It also generates a very similar syntax to when you use an arrow function as a type, which might be confusing, for example, this construct:

const MyComponentWithLongName: (
  arg: React.VoidFunctionComponent<MyComponentWithLongNameProps>,
) => any = ({ x, y }) => {
  const a = useA();
  return (
    <div>
      x = {x}; y = {y}; a = {a}
    </div>
  );
};

[kachkaev]

@thorn0 and folks, how strongly do you feel about using canBreak (as proposed) vs hacking a doc (as implemented)? I wonder if we can merge the PR as based on the test snapshots and then create a new PR which amends the implementation detail 🤔

This can help us make the 2.4.0 cut (this PR is the last one in the milestone).

[kachkaev]

This PR is replaced by #11515 👈 👀