diff --git a/changelog_unreleased/typescript/10024.md b/changelog_unreleased/typescript/10024.md index 26b71f780..eca023a3c 100644 --- a/changelog_unreleased/typescript/10024.md +++ b/changelog_unreleased/typescript/10024.md @@ -1,4 +1,4 @@ -#### Fix unnecessary line breaks in method type declaration parameters (#10024 by @sosukesuzuki) +#### Fix unnecessary line breaks in method type declaration parameters (#10024 by @sosukesuzuki, #10357 by @thorn0) ```js diff --git a/src/language-js/print/function-parameters.js b/src/language-js/print/function-parameters.js index 6841d282f..7a5b0150d 100644 --- a/src/language-js/print/function-parameters.js +++ b/src/language-js/print/function-parameters.js @@ -4,7 +4,7 @@ const { getNextNonSpaceNonCommentCharacter } = require("../../common/util"); const { printDanglingComments } = require("../../main/comments"); const { builders: { line, hardline, softline, group, indent, ifBreak }, - utils: { removeLines }, + utils: { removeLines, willBreak }, } = require("../../document"); const { getFunctionParameters, @@ -189,4 +189,48 @@ function shouldHugFunctionParameters(node) { ); } -module.exports = { printFunctionParameters, shouldHugFunctionParameters }; +function getReturnTypeNode(functionNode) { + let returnTypeNode; + if (functionNode.returnType) { + returnTypeNode = functionNode.returnType; + if (returnTypeNode.typeAnnotation) { + returnTypeNode = returnTypeNode.typeAnnotation; + } + } else if (functionNode.typeAnnotation) { + returnTypeNode = functionNode.typeAnnotation; + } + return returnTypeNode; +} + +// When parameters are grouped, the return type annotation breaks first. +function shouldGroupFunctionParameters(functionNode, returnTypeDoc) { + const returnTypeNode = getReturnTypeNode(functionNode); + if (!returnTypeNode) { + return false; + } + + const typeParameters = + functionNode.typeParameters && functionNode.typeParameters.params; + if (typeParameters) { + if (typeParameters.length > 1) { + return false; + } + if (typeParameters.length === 1) { + const typeParameter = typeParameters[0]; + if (typeParameter.constraint || typeParameter.default) { + return false; + } + } + } + + return ( + getFunctionParameters(functionNode).length === 1 && + (isObjectType(returnTypeNode) || willBreak(returnTypeDoc)) + ); +} + +module.exports = { + printFunctionParameters, + shouldHugFunctionParameters, + shouldGroupFunctionParameters, +}; diff --git a/src/language-js/print/function.js b/src/language-js/print/function.js index 745bfe57d..1cd092e42 100644 --- a/src/language-js/print/function.js +++ b/src/language-js/print/function.js @@ -25,7 +25,10 @@ const { CommentCheckFlags, } = require("../utils"); const { locEnd } = require("../loc"); -const { printFunctionParameters } = require("./function-parameters"); +const { + printFunctionParameters, + shouldGroupFunctionParameters, +} = require("./function-parameters"); const { printPropertyKey } = require("./property"); const { printFunctionTypeParameters } = require("./misc"); @@ -53,11 +56,20 @@ function printFunctionDeclaration(path, print, options, expandArg) { parts.push(path.call(print, "id")); } + const parametersDoc = printFunctionParameters( + path, + print, + options, + expandArg + ); + const returnTypeDoc = printReturnType(path, print, options); + const shouldGroupParameters = shouldGroupFunctionParameters(n, returnTypeDoc); + parts.push( printFunctionTypeParameters(path, options, print), group([ - printFunctionParameters(path, print, options, expandArg), - printReturnType(path, print, options), + shouldGroupParameters ? group(parametersDoc) : parametersDoc, + returnTypeDoc, ]), n.body ? " " : "", path.call(print, "body") @@ -110,15 +122,22 @@ function printMethod(path, options, print) { } function printMethodInternal(path, options, print) { + const node = path.getNode(); + const parametersDoc = printFunctionParameters(path, print, options); + const returnTypeDoc = printReturnType(path, print, options); + const shouldGroupParameters = shouldGroupFunctionParameters( + node, + returnTypeDoc + ); const parts = [ printFunctionTypeParameters(path, options, print), group([ - printFunctionParameters(path, print, options), - printReturnType(path, print, options), + shouldGroupParameters ? group(parametersDoc) : parametersDoc, + returnTypeDoc, ]), ]; - if (path.getNode().body) { + if (node.body) { parts.push(" ", path.call(print, "body")); } else { parts.push(options.semi ? ";" : ""); diff --git a/src/language-js/print/type-annotation.js b/src/language-js/print/type-annotation.js index db2245b89..59b9cbd29 100644 --- a/src/language-js/print/type-annotation.js +++ b/src/language-js/print/type-annotation.js @@ -15,7 +15,10 @@ const { shouldPrintComma, } = require("../utils"); const { printAssignment } = require("./assignment"); -const { printFunctionParameters } = require("./function-parameters"); +const { + printFunctionParameters, + shouldGroupFunctionParameters, +} = require("./function-parameters"); const { printArrayItems } = require("./array"); function shouldHugType(node) { @@ -244,26 +247,34 @@ function printFunctionType(path, options, print) { parts.push("("); } - parts.push( - printFunctionParameters( - path, - print, - options, - /* expandArg */ false, - /* printTypeParams */ true - ) + const parametersDoc = printFunctionParameters( + path, + print, + options, + /* expandArg */ false, + /* printTypeParams */ true ); // The returnType is not wrapped in a TypeAnnotation, so the colon // needs to be added separately. - if (n.returnType || n.predicate || n.typeAnnotation) { - parts.push( - isArrowFunctionTypeAnnotation ? " => " : ": ", - path.call(print, "returnType"), - path.call(print, "predicate"), - path.call(print, "typeAnnotation") - ); + const returnTypeDoc = + n.returnType || n.predicate || n.typeAnnotation + ? [ + isArrowFunctionTypeAnnotation ? " => " : ": ", + path.call(print, "returnType"), + path.call(print, "predicate"), + path.call(print, "typeAnnotation"), + ] + : ""; + + const shouldGroupParameters = shouldGroupFunctionParameters(n, returnTypeDoc); + + parts.push(shouldGroupParameters ? group(parametersDoc) : parametersDoc); + + if (returnTypeDoc) { + parts.push(returnTypeDoc); } + if (needsParens) { parts.push(")"); } diff --git a/src/language-js/print/typescript.js b/src/language-js/print/typescript.js index bea4104ce..b88e4e1a6 100644 --- a/src/language-js/print/typescript.js +++ b/src/language-js/print/typescript.js @@ -25,7 +25,10 @@ const { locStart, locEnd } = require("../loc"); const { printOptionalToken, printTypeScriptModifiers } = require("./misc"); const { printTernary } = require("./ternary"); -const { printFunctionParameters } = require("./function-parameters"); +const { + printFunctionParameters, + shouldGroupFunctionParameters, +} = require("./function-parameters"); const { printTemplateLiteral } = require("./template-literal"); const { printArrayItems } = require("./array"); const { printObject } = require("./object"); @@ -349,37 +352,46 @@ function printTypescript(path, options, print) { { shouldBreak } ); } - case "TSMethodSignature": + case "TSMethodSignature": { parts.push( - group([ - n.accessibility ? [n.accessibility, " "] : "", - n.export ? "export " : "", - n.static ? "static " : "", - n.readonly ? "readonly " : "", - n.computed ? "[" : "", - path.call(print, "key"), - n.computed ? "]" : "", - printOptionalToken(path), - printFunctionParameters( - path, - print, - options, - /* expandArg */ false, - /* printTypeParams */ true - ), - ]) + n.accessibility ? [n.accessibility, " "] : "", + n.export ? "export " : "", + n.static ? "static " : "", + n.readonly ? "readonly " : "", + n.computed ? "[" : "", + path.call(print, "key"), + n.computed ? "]" : "", + printOptionalToken(path) ); - if (n.returnType || n.typeAnnotation) { - parts.push( - group([ - ": ", - path.call(print, "returnType"), - path.call(print, "typeAnnotation"), - ]) - ); + const parametersDoc = printFunctionParameters( + path, + print, + options, + /* expandArg */ false, + /* printTypeParams */ true + ); + + const returnTypePropertyName = n.returnType + ? "returnType" + : "typeAnnotation"; + const returnTypeNode = n[returnTypePropertyName]; + const returnTypeDoc = returnTypeNode + ? path.call(print, returnTypePropertyName) + : ""; + const shouldGroupParameters = shouldGroupFunctionParameters( + n, + returnTypeDoc + ); + + parts.push(shouldGroupParameters ? group(parametersDoc) : parametersDoc); + + if (returnTypeNode) { + parts.push(": ", group(returnTypeDoc)); } + return group(parts); + } case "TSNamespaceExportDeclaration": parts.push("export as namespace ", path.call(print, "id")); diff --git a/tests/flow-repo/dictionary/__snapshots__/jsfmt.spec.js.snap b/tests/flow-repo/dictionary/__snapshots__/jsfmt.spec.js.snap index 4e23246fb..6d15b8a65 100644 --- a/tests/flow-repo/dictionary/__snapshots__/jsfmt.spec.js.snap +++ b/tests/flow-repo/dictionary/__snapshots__/jsfmt.spec.js.snap @@ -56,9 +56,10 @@ function foo0( return x; } -function foo2(x: { +function foo2(x: { [key: string]: number }): { [key: string]: number, -}): { [key: string]: number, +toString: () => string } { + +toString: () => string, +} { // x's prototype has a toString method return x; } @@ -468,9 +469,10 @@ function mix_with_declared_props(o: { [k: number]: X, p: Y }, x: X, y: Y) { } // Indeed, dict types are still Objects and have Object.prototype stuff -function object_prototype(o: { +function object_prototype(o: { [k: string]: number }): { [k: string]: number, -}): { [k: string]: number, +toString: () => string } { + +toString: () => string, +} { (o.toString(): boolean); // error: string ~> boolean return o; // ok } @@ -776,9 +778,10 @@ function foo3(x: { [key: string]: number }): { foo: number } { } // error: foo can't exist in x -function foo4(x: { +function foo4(x: { [key: string]: number }): { [key: string]: number, -}): { [key: string]: number, foo: string } { + foo: string, +} { return x; } diff --git a/tests/flow-repo/missing_annotation/__snapshots__/jsfmt.spec.js.snap b/tests/flow-repo/missing_annotation/__snapshots__/jsfmt.spec.js.snap index 5a14b8fc2..5637cc40b 100644 --- a/tests/flow-repo/missing_annotation/__snapshots__/jsfmt.spec.js.snap +++ b/tests/flow-repo/missing_annotation/__snapshots__/jsfmt.spec.js.snap @@ -132,9 +132,7 @@ var Foo = { }; }, - d: function ( - arg: string - ): { + d: function (arg: string): { bar: string, } { return { diff --git a/tests/flow-repo/spread/__snapshots__/jsfmt.spec.js.snap b/tests/flow-repo/spread/__snapshots__/jsfmt.spec.js.snap index 81b05c5d0..f064aed9a 100644 --- a/tests/flow-repo/spread/__snapshots__/jsfmt.spec.js.snap +++ b/tests/flow-repo/spread/__snapshots__/jsfmt.spec.js.snap @@ -46,9 +46,7 @@ function parseCounter(line: string): number { return 0; } -function parseGroup( - lines: Array -): { +function parseGroup(lines: Array): { counter: number, begin: number, end: number, diff --git a/tests/flow/method/__snapshots__/jsfmt.spec.js.snap b/tests/flow/method/__snapshots__/jsfmt.spec.js.snap index fc9bddf33..b67945453 100644 --- a/tests/flow/method/__snapshots__/jsfmt.spec.js.snap +++ b/tests/flow/method/__snapshots__/jsfmt.spec.js.snap @@ -24,6 +24,63 @@ type Foo = { ================================================================================ `; +exports[`consistent-breaking.js format 1`] = ` +====================================options===================================== +parsers: ["flow", "babel"] +printWidth: 80 + | printWidth +=====================================input====================================== +class Foo1 { + flowParseFunctionTypeParams( + params: N.FlowFunctionTypeParam[] = [], + ): { params: N.FlowFunctionTypeParam[], rest: ?N.FlowFunctionTypeParam } { + // ... + } +} + +type Foo2 = { + flowParseFunctionTypeParams( + params: N.FlowFunctionTypeParam[] + ): { params: N.FlowFunctionTypeParam[], rest: ?N.FlowFunctionTypeParam } +} + +{ + function flowParseFunctionTypeParams( + params: N.FlowFunctionTypeParam[] + ): { params: N.FlowFunctionTypeParam[], rest: ?N.FlowFunctionTypeParam } { + // ... + } +} + +=====================================output===================================== +class Foo1 { + flowParseFunctionTypeParams(params: N.FlowFunctionTypeParam[] = []): { + params: N.FlowFunctionTypeParam[], + rest: ?N.FlowFunctionTypeParam, + } { + // ... + } +} + +type Foo2 = { + flowParseFunctionTypeParams(params: N.FlowFunctionTypeParam[]): { + params: N.FlowFunctionTypeParam[], + rest: ?N.FlowFunctionTypeParam, + }, +}; + +{ + function flowParseFunctionTypeParams(params: N.FlowFunctionTypeParam[]): { + params: N.FlowFunctionTypeParam[], + rest: ?N.FlowFunctionTypeParam, + } { + // ... + } +} + +================================================================================ +`; + exports[`method.js format 1`] = ` ====================================options===================================== parsers: ["flow", "babel"] diff --git a/tests/flow/method/consistent-breaking.js b/tests/flow/method/consistent-breaking.js new file mode 100644 index 000000000..d5747c05f --- /dev/null +++ b/tests/flow/method/consistent-breaking.js @@ -0,0 +1,21 @@ +class Foo1 { + flowParseFunctionTypeParams( + params: N.FlowFunctionTypeParam[] = [], + ): { params: N.FlowFunctionTypeParam[], rest: ?N.FlowFunctionTypeParam } { + // ... + } +} + +type Foo2 = { + flowParseFunctionTypeParams( + params: N.FlowFunctionTypeParam[] + ): { params: N.FlowFunctionTypeParam[], rest: ?N.FlowFunctionTypeParam } +} + +{ + function flowParseFunctionTypeParams( + params: N.FlowFunctionTypeParam[] + ): { params: N.FlowFunctionTypeParam[], rest: ?N.FlowFunctionTypeParam } { + // ... + } +} diff --git a/tests/typescript/generic/__snapshots__/jsfmt.spec.js.snap b/tests/typescript/generic/__snapshots__/jsfmt.spec.js.snap index 38e2ba125..e6a52f240 100644 --- a/tests/typescript/generic/__snapshots__/jsfmt.spec.js.snap +++ b/tests/typescript/generic/__snapshots__/jsfmt.spec.js.snap @@ -181,3 +181,43 @@ export default { ================================================================================ `; + +exports[`ungrouped-parameters.ts format 1`] = ` +====================================options===================================== +parsers: ["typescript"] +printWidth: 80 + | printWidth +=====================================input====================================== +function filterTooltipWithFoo(oldEncoding: Encoding): { + customTooltipWithoutAggregatedField?: + | StringFieldDefWithCondition + | StringValueDefWithCondition + | StringFieldDef[]; + filteredEncoding: Encoding; +} { + const {tooltip, ...filteredEncoding} = oldEncoding; + if (!tooltip) { + return {filteredEncoding}; + } + // ... +} + +=====================================output===================================== +function filterTooltipWithFoo( + oldEncoding: Encoding +): { + customTooltipWithoutAggregatedField?: + | StringFieldDefWithCondition + | StringValueDefWithCondition + | StringFieldDef[]; + filteredEncoding: Encoding; +} { + const { tooltip, ...filteredEncoding } = oldEncoding; + if (!tooltip) { + return { filteredEncoding }; + } + // ... +} + +================================================================================ +`; diff --git a/tests/typescript/generic/ungrouped-parameters.ts b/tests/typescript/generic/ungrouped-parameters.ts new file mode 100644 index 000000000..063700bd7 --- /dev/null +++ b/tests/typescript/generic/ungrouped-parameters.ts @@ -0,0 +1,13 @@ +function filterTooltipWithFoo(oldEncoding: Encoding): { + customTooltipWithoutAggregatedField?: + | StringFieldDefWithCondition + | StringValueDefWithCondition + | StringFieldDef[]; + filteredEncoding: Encoding; +} { + const {tooltip, ...filteredEncoding} = oldEncoding; + if (!tooltip) { + return {filteredEncoding}; + } + // ... +} diff --git a/tests/typescript/method/__snapshots__/jsfmt.spec.js.snap b/tests/typescript/method/__snapshots__/jsfmt.spec.js.snap index 1693b0b0f..a444d144e 100644 --- a/tests/typescript/method/__snapshots__/jsfmt.spec.js.snap +++ b/tests/typescript/method/__snapshots__/jsfmt.spec.js.snap @@ -1,5 +1,55 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`issue-10352-consistency.ts format 1`] = ` +====================================options===================================== +parsers: ["typescript"] +printWidth: 80 + | printWidth +=====================================input====================================== +export interface Store { + getRecord(collectionName: string, documentPath: string): TaskEither>; +} + +export default class StoreImpl extends Service implements Store { + getRecord(collectionName: string, documentPath: string): TaskEither> { + // Do some stuff. + } +} + +export function loadPlugin( + name: string, + dirname: string, +): { filepath: string, value: mixed } { + // ... +} + +=====================================output===================================== +export interface Store { + getRecord( + collectionName: string, + documentPath: string + ): TaskEither>; +} + +export default class StoreImpl extends Service implements Store { + getRecord( + collectionName: string, + documentPath: string + ): TaskEither> { + // Do some stuff. + } +} + +export function loadPlugin( + name: string, + dirname: string +): { filepath: string; value: mixed } { + // ... +} + +================================================================================ +`; + exports[`method-signature.ts format 1`] = ` ====================================options===================================== parsers: ["typescript"] @@ -44,6 +94,40 @@ type Bar = { ================================================================================ `; +exports[`method-signature-with-wrapped-return-type.ts format 1`] = ` +====================================options===================================== +parsers: ["typescript"] +printWidth: 80 + | printWidth +=====================================input====================================== +type ReleaseToolConfig = { + get(key: "changelog"): { + get(key: "repo"): string; + get(key: "labels"): Map; + }; +}; + +type ReleaseToolConfig2 = { + get(key: "changelog"): \` + \` +}; + +=====================================output===================================== +type ReleaseToolConfig = { + get(key: "changelog"): { + get(key: "repo"): string; + get(key: "labels"): Map; + }; +}; + +type ReleaseToolConfig2 = { + get(key: "changelog"): \` + \`; +}; + +================================================================================ +`; + exports[`semi.ts format 1`] = ` ====================================options===================================== parsers: ["typescript"] diff --git a/tests/typescript/method/issue-10352-consistency.ts b/tests/typescript/method/issue-10352-consistency.ts new file mode 100644 index 000000000..691b39d23 --- /dev/null +++ b/tests/typescript/method/issue-10352-consistency.ts @@ -0,0 +1,16 @@ +export interface Store { + getRecord(collectionName: string, documentPath: string): TaskEither>; +} + +export default class StoreImpl extends Service implements Store { + getRecord(collectionName: string, documentPath: string): TaskEither> { + // Do some stuff. + } +} + +export function loadPlugin( + name: string, + dirname: string, +): { filepath: string, value: mixed } { + // ... +} diff --git a/tests/typescript/method/method-signature-with-wrapped-return-type.ts b/tests/typescript/method/method-signature-with-wrapped-return-type.ts new file mode 100644 index 000000000..a7a320693 --- /dev/null +++ b/tests/typescript/method/method-signature-with-wrapped-return-type.ts @@ -0,0 +1,11 @@ +type ReleaseToolConfig = { + get(key: "changelog"): { + get(key: "repo"): string; + get(key: "labels"): Map; + }; +}; + +type ReleaseToolConfig2 = { + get(key: "changelog"): ` + ` +};