Repository: prettier/prettier (JavaScript)

Issue:
The offset for React component body depends on component name length in v2.3
**Prettier 2.3.0**
[Playground link](https://prettier.io/playground/#N4Igxg9gdgLgprEAuEBLWcBOAzAhmOAAgFkBPAYQgFsAHaBGABUwhoGdDgAdKQwgDySEoAVyoAjLDwC+PHpChsYJCtTpQGQgEpx8MAHQA1CKgAmAMRFQwMVNEq16sADxkH6hs1ZsAfIQC8hAAUwAKE0gCUAX7cvIQKSoS4AYQibHAAgkERPHyYcDAimLzOpqgAbj78KcD80gDcSTW40s4A9GWVMnJQ6PA4+ERuak4wAExe7Jy5AkKiElJxpHNikpjdUPLQicOOGrBj2ro2RiYWVjZ2UO6jrqp7DBMs7H6BIQIANISk4VH+MTMEspkoE0plsjN8oVioRShUqjU6o0foFgKQGk1US12p0fBseH0sHgCCobvsYAB1VAwAAWABloABzAByuCocEmHFifEEwlWiz4yz5C3WUFkm2s22Uuw8sCptIZUBZbLgRz0pzMlmstnsI3JdzJDHl9KZrPZnNewVC-C+P0i0WmcSBmNS6SyOTiUKKJVx1VRSO+NXRjRBnGxHXhMhAHxArB1imQoFwmBYAHdGMmEGxkCBcAAbVO4UjZmPiTD4ADWBQAyjR8OhGcgYJgRHAY3AFqZTHBTHTcEqRLhGXBzBBMFRcDBbEqc7gRDAINGQDSYFQ8xSadS4Gw6wRq1nqRVqaQc2A2CW0IosExy4yJ8g8Hn0jGAFZsfgAIXLYCrMGrKrpdA4AffNnxAN9+GrBs8zgABFEQIHgECnzbEA60wdJMBzGBSBobcwEwVAaBgJcaEIuUzFpZAAA4AAYYzIiB0gpcsaBzMjtywcpgJjABHBD4E5Wc2AAWg0HseyXfJ+NQfJmCHe8kEfMD0ioVAmxbVC2GguCBOApTQNQmBcHEKlTCopAxhjZtcFQPMGwcRSQG3ABWJcwQAFRM7MDJQmNylbABJKBu1gasCKImAMhC6scJg5D0mkaQgA)
<!-- prettier-ignore -->
```sh
--parser typescript
```

After switching from Prettier 2.2.1 to 2.3.0 in a large React + TS codebase, I've noticed an unexpectedly large diff. The offset for many React components has shifted.

It turns out that the length of the component name now affects the location of `({`. If these opening brackets are moved to the new line, the whole component body is shifted. This makes git diffs quite fragile.

Imagine renaming some component during a refactor. In Prettier 2.2.1, the diff would be just one line but in Prettier 2.3.0, `git blame` for the whole file would end up broken.

**Input:**
<!-- prettier-ignore -->
```tsx
interface MyComponentProps {
  x: number
}

const MyComponent: React.VoidFunctionComponent<MyComponentProps> = ({ x }) => {
  const a = useA()
  return <div>x = {x}; a = {a}</div>
}

interface MyComponent2Props {
  x: number
  y: number
}

const MyComponent2: React.VoidFunctionComponent<MyComponent2Props> = ({ x, y }) => {
  const a = useA()
  return <div>x = {x}; y = {y}; a = {a}</div>
}

interface MyComponentWithLongNameProps {
  x: number
  y: number
}

const MyComponentWithLongName: React.VoidFunctionComponent<MyComponentWithLongNameProps> = ({ x, y }) => {
  const a = useA()
  return <div>x = {x}; y = {y}; a = {a}</div>
}
```

**Output:**
<!-- prettier-ignore -->
```tsx
interface MyComponentProps {
  x: number;
}

const MyComponent: React.VoidFunctionComponent<MyComponentProps> = ({ x }) => {
  const a = useA();
  return (
    <div>
      x = {x}; a = {a}
    </div>
  );
};

interface MyComponent2Props {
  x: number;
  y: number;
}

const MyComponent2: React.VoidFunctionComponent<MyComponent2Props> = ({
  x,
  y,
}) => {
  const a = useA();
  return (
    <div>
      x = {x}; y = {y}; a = {a}
    </div>
  );
};

interface MyComponentWithLongNameProps {
  x: number;
  y: number;
}

const MyComponentWithLongName: React.VoidFunctionComponent<MyComponentWithLongNameProps> =
  ({ x, y }) => {
    const a = useA();
    return (
      <div>
        x = {x}; y = {y}; a = {a}
      </div>
    );
  };

```

**Expected behavior:**

All three `useA()` and `return` statements have only two leading spaces, i.e. the offset does not depend on the component name length.

Our team will be staying on Prettier 2.2.1 in the meantime. Happy to help with a PR if we come up with some solution!


Hints:
Just checked if the issue is present in pure JavaScript and could reproduce it too: [Playgound](https://prettier.io/playground/#N4Igxg9gdgLgprEAuc0DOMAEBZAngYQgFsAHaBLAXkwAphMAPTAXwEpNKA+TYAHSkyZIUDJgCGHTAFc0cAII1W-QQCc4MKSoEAeACYBLAG6cm1YA2YBucZOBjm2gPQHj-Zv37DReQqXKwAdX0YAAsANTgVXAAZaABzADkxInk5NPSMzKzsnMzJOkYAGkxcFnYuHmUhdCwJahl5RSq1DS1MPSMTWwtrUrNcKxszeycXTjcPKC8sH2IyKAog0Iio2KhE5NTc7Z2d-PoGYtK2Dm4+AWqRWskGhSULls0dMdMeHpLbAes6nhHnTrcIEKIAgJBg+nQyFAYhUKggAHcAAowhBoZAgMQAG3hYlwaOBACMVGIwABrdQAZRIJP062QMBUUjgwLgRAJcF0ug50TE6ykYjicAAYhAVEQxDBwXSUGIpDAIECQCEYERMQEQsE4GhqWA4BTUcEjMFcOiwGh8SBabIVDBEcS4uLkAAzLGyYEAKzQDAAQsSyZTNtFaXBna7mSBPQwKbS4pi4ABFKQQeChzFukDUlTW9EEsTszGKkgqWkwIK6ULIAAcAAZgUWILIAsSSOii1rIoYQ8CAI5J+B20FomVoAC0Cw5HMVal7+jUdoFjqQLrT4dkRH09MZq5jccTyZDS7DwJgebLFaQACZj8T9JiY75FyAtQBWRUNAAqeaHy-ThiZAEkoC5WAKTAYswTkICKRgXA41TWRmGYIA).

Of course, the name of the component should be quite long to shift the fn body 😅 The TS example is much closer to the real world scenario. 
Is `git blame` the main concern here? It has the `-w` mode, in which it ignores whitespace changes. Also it has the [`--ignore-revs-file` feature](https://git-scm.com/docs/git-blame#Documentation/git-blame.txt---ignore-revs-fileltfilegt). Don't these Git features solve this problem completely?
Said that, I agree that it's better to minimize indentation when possible. For type-annotated variables, the following formatting seems to be an interesting idea:

```ts
const MyComponentWithLongName: (
  React.VoidFunctonComponent<MyComponentWithLongNameProps>
) = ({ x, y }) => {
  const a = useA();
  return (
    <div>
      x = {x}; y = {y}; a = {a}
    </div>
  );
};
```

But for variables without type annotations, I'm afraid the current (2.3) formatting is going to stay since it's difficult to think of an alternative.
Git blame is one of the two concerns I have. Just checked what [VSCode git lens](https://marketplace.visualstudio.com/items?itemName=eamodio.gitlens) shows for the updated lines and saw the commit with an offset shift rather than the original one. This makes code introspection more challenging than it used to. Even if it’s technically possible to look ‘through’ the whitespace-only diffs, it's not a default setting, so requires an extra action from the whole team.

Another concern is the ‘broken rhythm’. I’d say that it’s even bigger than `git blame`.

Human brain unconsciously gets used to the standard offsets in the React files, which helps certain cognitive tasks. Determining if a given line is inside a hook or in the plain 'render phase' is a typical example. If there's no ‘rule of thumb‘ for the whole component body offset, this mini-task requires an extra split second hundreds of times a day.
> `gitlens.blame.ignoreWhitespace` Specifies whether to ignore whitespace when comparing revisions during blame operations

-- https://marketplace.visualstudio.com/items?itemName=eamodio.gitlens#gutter-blame-settings-
If we're talking specifically about React, I might be missing something. Why are these type annotations needed? In my React projects, I annotate only the props parameter and let TS infer the type of the variable. (Also I use function declarations instead of `const`s because I think hoisting is convenient.) Anyway, I guess you have your reasons for doing it that way, so if you feel like implementing https://github.com/prettier/prettier/issues/10848#issuecomment-836828099, please go ahead. I think it'd have good chances of getting merged.
Thanks @thorn0 – I might give that PR a shot soon.

I think I understand what you mean by using `function` keyword and by typing props only. In that case we get something like:

```tsx
interface MyComponentWithLongNameProps {
  x: number;
  y: number;
}

function MyComponentWithLongName({ x, y }: MyComponentWithLongNameProps) {
  const a = useA();
  return (
    <div>
      x = {x}; y = {y}; a = {a}
    </div>
  );
}
```

This is a rather wide-spread approach, but we don’t use it for these two reasons:

- Unlike `function myVar`, `const myVar = ...` is universal. By using [`"func-style": "error"` in ESLint](https://eslint.org/docs/rules/func-style), we enforce the expression fn style and this saves the team from a number of pitfalls and avoidable discussions. The difference between the two styles includes pretty nasty traps for newcomers, so it's best to not have to pick between one style and the other.

  Not having hoisting can be seen as a good thing. It helps developers stick with a coherent ‘rhythm’ and thus write code that can be read linearly.

- [`React.VoidFunctonComponent`](https://github.com/DefinitelyTyped/DefinitelyTyped/pull/46643) checks the return type, not just the args. So it's harder to return `undefined` or `void` by accident. When using `function` + `props: MyProps`, the return type will be also checked, but only when the component is mentioned in some Jsx. This works too, but makes things a bit more confusing for the devs.

  If `undefined` is returned in a component library, the typing issue might be only spotted after the release.

  `React.VoidFunctonComponent` is also ‘symmetric’ with `React.ForwardRefRenderFunction`, which is an extra bonus.

Overall, with `const` + `React.VoidFunctonComponent`, there are fewer places that need to look bespoke. Thus, the ‘list of rules’ to follow is shorter. Human brains like that 🙂
@thorn0 WDYT of 

```ts
const MyComponentWithLongName: React.VoidFunctonComponent<MyComponentWithLongNameProps> = ({
  x,
  y,
}) => {
```

instead of 

```ts
const MyComponentWithLongName: (
  React.VoidFunctonComponent<MyComponentWithLongNameProps>
) = ({ x, y }) => {
```

?

So my third example would work as the second one. The benefit I see here is that `const MyComponent = React.VoidFunctonComponent<MyComponentProps>` will always be on one line. This will make it easier for the brain to pattern-match.  Sticking with one line despite its length is what Prettier 2.2.1 does already.
This line is too long. The fact that Prettier doesn't break it is kind of a bug. Also Prettier's general strategy with regard to arrow functions is "prefer to keep the signature on one line if possible".
Sorry for the lack of updates @thorn0. I’m still planning to look into [your suggestion](https://github.com/prettier/prettier/issues/10848#issuecomment-836828099) – just haven’t found time yet. When 2.3.1 shows up on the horizon, please let me know and I’ll prioritise the work!

If anyone else wants to submit a PR, that’ll be amazing!
Now that TS 4.3 got released, we need to release support for it, so Prettier 2.3.1 is near.
Thanks for the headsup @thorn0! I'm planning to look into the fix now. If you have any hints / technical suggestions, it'd be great if you could share them!


First of all, you'll need to detect this situation in the code that prints assignment-like constructions (`print/assignment.js`) and switch the _layout_ to `break-lhs`. Then modify the code that prints type annotations in variable declarations.

Patch:
diff --git a/src/document/doc-utils.js b/src/document/doc-utils.js
--- a/src/document/doc-utils.js
+++ b/src/document/doc-utils.js
@@ -401,6 +401,16 @@ function replaceTextEndOfLine(text, replacement = literalline) {
   return join(replacement, text.split("\n")).parts;
 }
 
+function canBreakFn(doc) {
+  if (doc.type === "line") {
+    return true;
+  }
+}
+
+function canBreak(doc) {
+  return findInDoc(doc, canBreakFn, false);
+}
+
 module.exports = {
   isConcat,
   getDocParts,
@@ -416,4 +426,5 @@ module.exports = {
   cleanDoc,
   replaceTextEndOfLine,
   replaceEndOfLine,
+  canBreak,
 };
diff --git a/src/language-js/print/assignment.js b/src/language-js/print/assignment.js
--- a/src/language-js/print/assignment.js
+++ b/src/language-js/print/assignment.js
@@ -3,7 +3,7 @@
 const { isNonEmptyArray, getStringWidth } = require("../../common/util.js");
 const {
   builders: { line, group, indent, indentIfBreak },
-  utils: { cleanDoc, willBreak },
+  utils: { cleanDoc, willBreak, canBreak },
 } = require("../../document/index.js");
 const {
   hasLeadingOwnLineComment,
@@ -140,7 +140,8 @@ function chooseLayout(path, options, print, leftDoc, rightPropertyName) {
   if (
     isComplexDestructuring(node) ||
     isComplexTypeAliasParams(node) ||
-    hasComplexTypeAnnotation(node)
+    hasComplexTypeAnnotation(node) ||
+    (isArrowFunctionVariableDeclarator(node) && canBreak(leftDoc))
   ) {
     return "break-lhs";
   }
@@ -295,6 +296,14 @@ function hasComplexTypeAnnotation(node) {
   );
 }
 
+function isArrowFunctionVariableDeclarator(node) {
+  return (
+    node.type === "VariableDeclarator" &&
+    node.init &&
+    node.init.type === "ArrowFunctionExpression"
+  );
+}
+
 function getTypeParametersFromTypeReference(node) {
   if (
     isTypeReference(node) &&
@@ -457,4 +466,5 @@ module.exports = {
   printVariableDeclarator,
   printAssignmentExpression,
   printAssignment,
+  isArrowFunctionVariableDeclarator,
 };
diff --git a/src/language-js/print/type-parameters.js b/src/language-js/print/type-parameters.js
--- a/src/language-js/print/type-parameters.js
+++ b/src/language-js/print/type-parameters.js
@@ -11,9 +11,11 @@ const {
   isTSXFile,
   shouldPrintComma,
   getFunctionParameters,
+  isObjectType,
 } = require("../utils.js");
 const { createGroupIdMapper } = require("../../common/util.js");
 const { shouldHugType } = require("./type-annotation.js");
+const { isArrowFunctionVariableDeclarator } = require("./assignment.js");
 
 const getTypeParametersGroupId = createGroupIdMapper("typeParameters");
 
@@ -32,12 +34,22 @@ function printTypeParameters(path, options, print, paramsKey) {
   const grandparent = path.getNode(2);
   const isParameterInTestCall = grandparent && isTestCall(grandparent);
 
+  const isArrowFunctionVariable = path.match(
+    (node) =>
+      !(node[paramsKey].length === 1 && isObjectType(node[paramsKey][0])),
+    undefined,
+    (node, name) => name === "typeAnnotation",
+    (node) => node.type === "Identifier",
+    isArrowFunctionVariableDeclarator
+  );
+
   const shouldInline =
-    isParameterInTestCall ||
-    node[paramsKey].length === 0 ||
-    (node[paramsKey].length === 1 &&
-      (shouldHugType(node[paramsKey][0]) ||
-        node[paramsKey][0].type === "NullableTypeAnnotation"));
+    !isArrowFunctionVariable &&
+    (isParameterInTestCall ||
+      node[paramsKey].length === 0 ||
+      (node[paramsKey].length === 1 &&
+        (node[paramsKey][0].type === "NullableTypeAnnotation" ||
+          shouldHugType(node[paramsKey][0]))));
 
   if (shouldInline) {
     return [


Test patch:
diff --git a/tests/format/typescript/assignment/__snapshots__/jsfmt.spec.js.snap b/tests/format/typescript/assignment/__snapshots__/jsfmt.spec.js.snap
--- a/tests/format/typescript/assignment/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/format/typescript/assignment/__snapshots__/jsfmt.spec.js.snap
@@ -263,6 +263,150 @@ const foo = call<
 ================================================================================
 `;
 
+exports[`issue-10848.tsx format 1`] = `
+====================================options=====================================
+parsers: ["typescript"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+const MyComponent: React.VoidFunctionComponent<MyComponentProps> = ({ x }) => {
+  const a = useA()
+  return <div>x = {x}; a = {a}</div>
+}
+
+const MyComponent2: React.VoidFunctionComponent<MyComponent2Props> = ({ x, y }) => {
+  const a = useA()
+  return <div>x = {x}; y = {y}; a = {a}</div>
+}
+
+const MyComponentWithLongName1: React.VoidFunctionComponent<MyComponentWithLongNameProps> = ({ x, y }) => {
+  const a = useA()
+  return <div>x = {x}; y = {y}; a = {a}</div>
+}
+
+const MyComponentWithLongName2: React.VoidFunctionComponent<MyComponentWithLongNameProps> = ({ x, y, anotherPropWithLongName1, anotherPropWithLongName2, anotherPropWithLongName3, anotherPropWithLongName4 }) => {
+  const a = useA()
+  return <div>x = {x}; y = {y}; a = {a}</div>
+}
+
+const MyGenericComponent: React.VoidFunctionComponent<MyGenericComponentProps<number>> = ({ x, y }) => {
+  const a = useA()
+  return <div>x = {x}; y = {y}; a = {a}</div>
+}
+
+export const ExportToExcalidrawPlus: React.FC<{
+  elements: readonly NonDeletedExcalidrawElement[];
+  appState: AppState;
+  onError: (error: Error) => void;
+}> = ({ elements, appState, onError }) => {
+  return null;
+}
+
+const Query: FunctionComponent<QueryProps> = ({
+    children,
+    type,
+    resource,
+    payload,
+    // Provides an undefined onSuccess just so the key \`onSuccess\` is defined
+    // This is used to detect options in useDataProvider
+    options = { onSuccess: undefined },
+}) =>
+    children(
+        useQuery(
+            { type, resource, payload },
+            { ...options, withDeclarativeSideEffectsSupport: true }
+        )
+    );
+
+=====================================output=====================================
+const MyComponent: React.VoidFunctionComponent<MyComponentProps> = ({ x }) => {
+  const a = useA();
+  return (
+    <div>
+      x = {x}; a = {a}
+    </div>
+  );
+};
+
+const MyComponent2: React.VoidFunctionComponent<MyComponent2Props> = ({
+  x,
+  y,
+}) => {
+  const a = useA();
+  return (
+    <div>
+      x = {x}; y = {y}; a = {a}
+    </div>
+  );
+};
+
+const MyComponentWithLongName1: React.VoidFunctionComponent<
+  MyComponentWithLongNameProps
+> = ({ x, y }) => {
+  const a = useA();
+  return (
+    <div>
+      x = {x}; y = {y}; a = {a}
+    </div>
+  );
+};
+
+const MyComponentWithLongName2: React.VoidFunctionComponent<
+  MyComponentWithLongNameProps
+> = ({
+  x,
+  y,
+  anotherPropWithLongName1,
+  anotherPropWithLongName2,
+  anotherPropWithLongName3,
+  anotherPropWithLongName4,
+}) => {
+  const a = useA();
+  return (
+    <div>
+      x = {x}; y = {y}; a = {a}
+    </div>
+  );
+};
+
+const MyGenericComponent: React.VoidFunctionComponent<
+  MyGenericComponentProps<number>
+> = ({ x, y }) => {
+  const a = useA();
+  return (
+    <div>
+      x = {x}; y = {y}; a = {a}
+    </div>
+  );
+};
+
+export const ExportToExcalidrawPlus: React.FC<{
+  elements: readonly NonDeletedExcalidrawElement[];
+  appState: AppState;
+  onError: (error: Error) => void;
+}> = ({ elements, appState, onError }) => {
+  return null;
+};
+
+const Query: FunctionComponent<QueryProps> = ({
+  children,
+  type,
+  resource,
+  payload,
+  // Provides an undefined onSuccess just so the key \`onSuccess\` is defined
+  // This is used to detect options in useDataProvider
+  options = { onSuccess: undefined },
+}) =>
+  children(
+    useQuery(
+      { type, resource, payload },
+      { ...options, withDeclarativeSideEffectsSupport: true }
+    )
+  );
+
+================================================================================
+`;
+
 exports[`issue-10850.ts format 1`] = `
 ====================================options=====================================
 parsers: ["typescript"]
diff --git a/tests/format/typescript/assignment/issue-10848.tsx b/tests/format/typescript/assignment/issue-10848.tsx
new file mode 100644
--- /dev/null
+++ b/tests/format/typescript/assignment/issue-10848.tsx
@@ -0,0 +1,48 @@
+const MyComponent: React.VoidFunctionComponent<MyComponentProps> = ({ x }) => {
+  const a = useA()
+  return <div>x = {x}; a = {a}</div>
+}
+
+const MyComponent2: React.VoidFunctionComponent<MyComponent2Props> = ({ x, y }) => {
+  const a = useA()
+  return <div>x = {x}; y = {y}; a = {a}</div>
+}
+
+const MyComponentWithLongName1: React.VoidFunctionComponent<MyComponentWithLongNameProps> = ({ x, y }) => {
+  const a = useA()
+  return <div>x = {x}; y = {y}; a = {a}</div>
+}
+
+const MyComponentWithLongName2: React.VoidFunctionComponent<MyComponentWithLongNameProps> = ({ x, y, anotherPropWithLongName1, anotherPropWithLongName2, anotherPropWithLongName3, anotherPropWithLongName4 }) => {
+  const a = useA()
+  return <div>x = {x}; y = {y}; a = {a}</div>
+}
+
+const MyGenericComponent: React.VoidFunctionComponent<MyGenericComponentProps<number>> = ({ x, y }) => {
+  const a = useA()
+  return <div>x = {x}; y = {y}; a = {a}</div>
+}
+
+export const ExportToExcalidrawPlus: React.FC<{
+  elements: readonly NonDeletedExcalidrawElement[];
+  appState: AppState;
+  onError: (error: Error) => void;
+}> = ({ elements, appState, onError }) => {
+  return null;
+}
+
+const Query: FunctionComponent<QueryProps> = ({
+    children,
+    type,
+    resource,
+    payload,
+    // Provides an undefined onSuccess just so the key `onSuccess` is defined
+    // This is used to detect options in useDataProvider
+    options = { onSuccess: undefined },
+}) =>
+    children(
+        useQuery(
+            { type, resource, payload },
+            { ...options, withDeclarativeSideEffectsSupport: true }
+        )
+    );