Fix trailing coma print in type prameters, improve jsx parse (#14688)

Co-authored-by: fisker Cheung <lionkay@gmail.com>

changelog_unreleased/typescript/14688.md

@@ -0,0 +1,15 @@
+#### Fix missing required comma in type parameters (#14688 by @fisker, @sosukesuzuki)
+
+Previously, we only print trailing comma when file extension is `.tsx`, turns out `.mts`, `.cts` files requires it to parse too.
+
+<!-- prettier-ignore -->
+```tsx
+// Input
+export const unsafeCoerce = <T,>(u: unknown): T => u as T
+
+// Prettier stable
+export const unsafeCoerce = <T>(u: unknown): T => u as T;
+
+// Prettier main
+export const unsafeCoerce = <T,>(u: unknown): T => u as T;
+```

scripts/build/config.js

@@ -185,7 +185,7 @@ const pluginFiles = [
         process: (text) =>
           text.replace(
             'require("path")',
-            "{extname: file => file.split('.').pop()}"
+            '{extname: file => "." + file.split(".").pop()}'
           ),
       },
       {

src/language-js/parse/typescript.js

@@ -7,7 +7,7 @@ import postprocess from "./postprocess/index.js";
 import { throwErrorForInvalidNodes } from "./postprocess/typescript.js";
 
 /** @type {import("@typescript-eslint/typescript-estree").TSESTreeOptions} */
-const parseOptions = {
+const baseParseOptions = {
   // `jest@<=26.4.2` rely on `loc`
   // https://github.com/facebook/jest/issues/10444
   // Set `loc` and `range` to `true` also prevent AST traverse
@@ -15,7 +15,6 @@ const parseOptions = {
   loc: true,
   range: true,
   comment: true,
-  jsx: true,
   tokens: true,
   loggerFn: false,
   project: [],
@@ -37,18 +36,34 @@ function createParseError(error) {
   });
 }
 
-function parse(text) {
+// https://typescript-eslint.io/architecture/parser/#jsx
+const isKnownFileType = (filepath) =>
+  /\.(?:js|mjs|cjs|jsx|ts|mts|cts|tsx)$/i.test(filepath);
+
+function getParseOptionsCombinations(text, options) {
+  const filepath = options?.filepath;
+  if (filepath && isKnownFileType(filepath)) {
+    return [{ ...baseParseOptions, filePath: filepath }];
+  }
+
+  const shouldEnableJsx = isProbablyJsx(text);
+  return [
+    { ...baseParseOptions, jsx: shouldEnableJsx },
+    { ...baseParseOptions, jsx: !shouldEnableJsx },
+  ];
+}
+
+function parse(text, options) {
   const textToParse = replaceHashbang(text);
-  const jsx = isProbablyJsx(text);
+  const parseOptionsCombinations = getParseOptionsCombinations(text, options);
 
   let result;
   try {
-    result = tryCombinations([
-      // Try passing with our best guess first.
-      () => parseWithNodeMaps(textToParse, { ...parseOptions, jsx }),
-      // But if we get it wrong, try the opposite.
-      () => parseWithNodeMaps(textToParse, { ...parseOptions, jsx: !jsx }),
-    ]);
+    result = tryCombinations(
+      parseOptionsCombinations.map(
+        (parseOptions) => () => parseWithNodeMaps(textToParse, parseOptions)
+      )
+    );
   } catch ({
     errors: [
       // Suppose our guess is correct, throw the first error

src/language-js/print/type-parameters.js

@@ -12,7 +12,6 @@ import {
   isTestCall,
   hasComment,
   CommentCheckFlags,
-  isTSXFile,
   shouldPrintComma,
   getFunctionParameters,
   isObjectType,
@@ -27,6 +26,20 @@ import { printTypeScriptMappedTypeModifier } from "./mapped-type.js";
 
 const getTypeParametersGroupId = createGroupIdMapper("typeParameters");
 
+// Keep comma if the file extension not `.ts` and
+// has one type parameter that isn't extend with any types.
+// Because, otherwise formatted result will be invalid as tsx.
+function shouldForceTrailingComma(path, options, paramsKey) {
+  const { node } = path;
+  return (
+    getFunctionParameters(node).length === 1 &&
+    node.type.startsWith("TS") &&
+    !node[paramsKey][0].constraint &&
+    path.parent.type === "ArrowFunctionExpression" &&
+    !(options.filepath && /\.ts$/.test(options.filepath))
+  );
+}
+
 function printTypeParameters(path, options, print, paramsKey) {
   const { node } = path;
 
@@ -68,16 +81,10 @@ function printTypeParameters(path, options, print, paramsKey) {
     ];
   }
 
-  // Keep comma if the file extension is .tsx and
-  // has one type parameter that isn't extend with any types.
-  // Because, otherwise formatted result will be invalid as tsx.
   const trailingComma =
     node.type === "TSTypeParameterInstantiation" // https://github.com/microsoft/TypeScript/issues/21984
       ? ""
-      : getFunctionParameters(node).length === 1 &&
-        isTSXFile(options) &&
-        !node[paramsKey][0].constraint &&
-        path.parent.type === "ArrowFunctionExpression"
+      : shouldForceTrailingComma(path, options, paramsKey)
       ? ","
       : shouldPrintComma(options)
       ? ifBreak(",")

src/language-js/utils/index.js

@@ -730,10 +730,6 @@ function identity(x) {
   return x;
 }
 
-function isTSXFile(options) {
-  return options.filepath && /\.tsx$/i.test(options.filepath);
-}
-
 /**
  * @param {any} options
  * @param {("es5" | "all")} [level]
@@ -1151,7 +1147,6 @@ export {
   isStringPropSafeToUnquote,
   isTemplateOnItsOwnLine,
   isTestCall,
-  isTSXFile,
   isTypeAnnotationAFunction,
   isNextLineEmpty,
   needsHardlineAfterDanglingComment,

tests/format/misc/errors/typescript/__snapshots__/jsfmt.spec.js.snap

@@ -32,7 +32,7 @@ exports[`export-declare.ts [typescript] format 1`] = `
   2 |"
 `;
 
-exports[`invalid-jsx-1.ts [typescript] format 1`] = `
+exports[`invalid-jsx-1.tsx [typescript] format 1`] = `
 "Unexpected token. Did you mean \`{'>'}\` or \`>\`? (3:45)
   1 | // https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-9.html#-and--are-now-invalid-jsx-text-characters
   2 |

tests/format/typescript/comments/__snapshots__/jsfmt.spec.js.snap

@@ -62,7 +62,7 @@ abstract class AbstractFoo {
 ================================================================================
 `;
 
-exports[`after_jsx_generic.ts format 1`] = `
+exports[`after_jsx_generic.tsx format 1`] = `
 ====================================options=====================================
 parsers: ["typescript"]
 printWidth: 80
@@ -293,7 +293,7 @@ export type AsyncExecuteOptions = child_process$execFileOpts & {
 ================================================================================
 `;
 
-exports[`jsx.ts format 1`] = `
+exports[`jsx.tsx format 1`] = `
 ====================================options=====================================
 parsers: ["typescript"]
 printWidth: 80

tests/format/typescript/last-argument-expansion/__snapshots__/jsfmt.spec.js.snap

@@ -38,7 +38,7 @@ export default class AddAssetHtmlPlugin {
 ================================================================================
 `;
 
-exports[`decorated-function.ts format 1`] = `
+exports[`decorated-function.tsx format 1`] = `
 ====================================options=====================================
 parsers: ["typescript"]
 printWidth: 80
@@ -198,7 +198,7 @@ var listener = DOM.listen(
 ================================================================================
 `;
 
-exports[`forward-ref.ts format 1`] = `
+exports[`forward-ref.tsx format 1`] = `
 ====================================options=====================================
 parsers: ["typescript"]
 printWidth: 80

tests/format/typescript/tsx/comma/__snapshots__/jsfmt.spec.js.snap

@@ -0,0 +1,131 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`snippet: test.cjs format 1`] = `
+====================================options=====================================
+parsers: ["typescript"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+const A = <T,>() => {}
+=====================================output=====================================
+const A = <T,>() => {};
+
+================================================================================
+`;
+
+exports[`snippet: test.cts format 1`] = `
+====================================options=====================================
+parsers: ["typescript"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+const A = <T,>() => {}
+=====================================output=====================================
+const A = <T,>() => {};
+
+================================================================================
+`;
+
+exports[`snippet: test.js format 1`] = `
+====================================options=====================================
+parsers: ["typescript"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+const A = <T,>() => {}
+=====================================output=====================================
+const A = <T,>() => {};
+
+================================================================================
+`;
+
+exports[`snippet: test.jsx format 1`] = `
+====================================options=====================================
+parsers: ["typescript"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+const A = <T,>() => {}
+=====================================output=====================================
+const A = <T,>() => {};
+
+================================================================================
+`;
+
+exports[`snippet: test.mjs format 1`] = `
+====================================options=====================================
+parsers: ["typescript"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+const A = <T,>() => {}
+=====================================output=====================================
+const A = <T,>() => {};
+
+================================================================================
+`;
+
+exports[`snippet: test.mts format 1`] = `
+====================================options=====================================
+parsers: ["typescript"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+const A = <T,>() => {}
+=====================================output=====================================
+const A = <T,>() => {};
+
+================================================================================
+`;
+
+exports[`snippet: test.ts format 1`] = `
+====================================options=====================================
+parsers: ["typescript"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+const A = <T,>() => {}
+=====================================output=====================================
+const A = <T>() => {};
+
+================================================================================
+`;
+
+exports[`snippet: test.tsx format 1`] = `
+====================================options=====================================
+parsers: ["typescript"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+const A = <T,>() => {}
+=====================================output=====================================
+const A = <T,>() => {};
+
+================================================================================
+`;
+
+exports[`snippet: test.unknown format 1`] = `
+====================================options=====================================
+parsers: ["typescript"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+const A = <T,>() => {}
+=====================================output=====================================
+const A = <T,>() => {};
+
+================================================================================
+`;
+
+exports[`snippet: unnamed format 1`] = `
+====================================options=====================================
+parsers: ["typescript"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+const A = <T,>() => {}
+=====================================output=====================================
+const A = <T,>() => {};
+
+================================================================================
+`;

tests/format/typescript/tsx/comma/jsfmt.spec.js

@@ -0,0 +1,20 @@
+const code = "const A = <T,>() => {}";
+
+run_spec(
+  {
+    importMeta: import.meta,
+    snippets: [
+      "test.js",
+      "test.cjs",
+      "test.mjs",
+      "test.ts",
+      "test.jsx",
+      "test.mts",
+      "test.cts",
+      "test.tsx",
+      "test.unknown",
+      undefined,
+    ].map((filename) => ({ code, filename, name: filename ?? "unnamed" })),
+  },
+  ["typescript"]
+);

tests/format/typescript/typeparams/print-width-120/__snapshots__/jsfmt.spec.js.snap

@@ -1,6 +1,6 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`issue-7542.ts - {"printWidth":120} format 1`] = `
+exports[`issue-7542.tsx - {"printWidth":120} format 1`] = `
 ====================================options=====================================
 parsers: ["typescript"]
 printWidth: 120

tests/integration/__tests__/format.js

@@ -24,7 +24,7 @@ test("typescript parser should throw the first error when both JSX and non-JSX m
     label:
   `;
   await expect(
-    prettier.format(input, { parser: "typescript" })
+    prettier.format(input, { parser: "typescript", filepath: "foo.unknown" })
   ).rejects.toThrowErrorMatchingSnapshot();
 });