diff --git a/changelog_unreleased/javascript/pr-7889.md b/changelog_unreleased/javascript/pr-7889.md new file mode 100644 index 000000000..bb2ccb166 --- /dev/null +++ b/changelog_unreleased/javascript/pr-7889.md @@ -0,0 +1,23 @@ +#### Further improve method chain breaking heuristic ([#7889](https://github.com/prettier/prettier/pull/7889) by [@thorn0](https://github.com/thorn0)) + +A method chain now is always split onto multiple lines if: + +- it's an expression statement and all its arguments are literals or `UPPERCASE_CONSTANT`s (the "fluent configuration" pattern), +- any call but the first one has more than two arguments, or +- the chain starts with a constructor call. + + +```jsx +// Input +cy.get(".ready") + .should("have.text", "FOO") + .should("have.css", "color", "#aaa"); + +// Prettier stable +cy.get(".ready").should("have.text", "FOO").should("have.css", "color", "#aaa"); + +// Prettier master +cy.get(".ready") + .should("have.text", "FOO") + .should("have.css", "color", "#aaa"); +``` diff --git a/src/language-js/print/member-chain.js b/src/language-js/print/member-chain.js index ea231aa56..19c68cc27 100644 --- a/src/language-js/print/member-chain.js +++ b/src/language-js/print/member-chain.js @@ -1,5 +1,7 @@ "use strict"; +const flat = require("lodash/flatten"); + const comments = require("../../main/comments"); const { getLast } = require("../../common/util"); const { @@ -12,7 +14,7 @@ const { hasLeadingComment, hasTrailingComment, isCallOrOptionalCallExpression, - isFunctionOrArrowExpression, + isLiteralLikeValue, isLongCurriedCallExpression, isMemberish, isNumericLiteral, @@ -52,6 +54,10 @@ const { // MemberExpression and CallExpression. We need to traverse the AST // and make groups out of it to print it in the desired way. function printMemberChain(path, options, print) { + const parent = path.getParentNode(); + const isExpressionStatement = + !parent || parent.type === "ExpressionStatement"; + // The first phase is to linearize the AST by traversing it down. // // a().b() @@ -88,11 +94,8 @@ function printMemberChain(path, options, print) { function rec(path) { const node = path.getValue(); if ( - (node.type === "CallExpression" || - node.type === "OptionalCallExpression") && - (isMemberish(node.callee) || - node.callee.type === "CallExpression" || - node.callee.type === "OptionalCallExpression") + isCallOrOptionalCallExpression(node) && + (isMemberish(node.callee) || isCallOrOptionalCallExpression(node.callee)) ) { printedNodes.unshift({ node, @@ -183,8 +186,7 @@ function printMemberChain(path, options, print) { for (; i < printedNodes.length; ++i) { if ( printedNodes[i].node.type === "TSNonNullExpression" || - printedNodes[i].node.type === "OptionalCallExpression" || - printedNodes[i].node.type === "CallExpression" || + isCallOrOptionalCallExpression(printedNodes[i].node) || ((printedNodes[i].node.type === "MemberExpression" || printedNodes[i].node.type === "OptionalMemberExpression") && printedNodes[i].node.computed && @@ -195,10 +197,7 @@ function printMemberChain(path, options, print) { break; } } - if ( - printedNodes[0].node.type !== "CallExpression" && - printedNodes[0].node.type !== "OptionalCallExpression" - ) { + if (!isCallOrOptionalCallExpression(printedNodes[0].node)) { for (; i + 1 < printedNodes.length; ++i) { if ( isMemberish(printedNodes[i].node) && @@ -235,10 +234,7 @@ function printMemberChain(path, options, print) { hasSeenCallExpression = false; } - if ( - printedNodes[i].node.type === "CallExpression" || - printedNodes[i].node.type === "OptionalCallExpression" - ) { + if (isCallOrOptionalCallExpression(printedNodes[i].node)) { hasSeenCallExpression = true; } currentGroup.push(printedNodes[i]); @@ -284,8 +280,6 @@ function printMemberChain(path, options, print) { } function shouldNotWrap(groups) { - const parent = path.getParentNode(); - const isExpression = parent && parent.type === "ExpressionStatement"; const hasComputed = groups[1].length && groups[1][0].node.computed; if (groups[0].length === 1) { @@ -294,7 +288,7 @@ function printMemberChain(path, options, print) { firstNode.type === "ThisExpression" || (firstNode.type === "Identifier" && (isFactory(firstNode.name) || - (isExpression && isShort(firstNode.name)) || + (isExpressionStatement && isShort(firstNode.name)) || hasComputed)) ); } @@ -337,7 +331,7 @@ function printMemberChain(path, options, print) { const oneLine = concat(printedGroups); const cutoff = shouldMerge ? 3 : 2; - const flatGroups = groups.reduce((res, group) => res.concat(group), []); + const flatGroups = flat(groups); const hasComment = flatGroups.slice(1, -1).some((node) => hasLeadingComment(node.node)) || @@ -355,17 +349,14 @@ function printMemberChain(path, options, print) { // Find out the last node in the first group and check if it has an // empty line after - const lastNodeBeforeIndent = getLast( - shouldMerge ? groups.slice(1, 2)[0] : groups[0] - ).node; + const lastNodeBeforeIndent = getLast(groups[shouldMerge ? 1 : 0]).node; const shouldHaveEmptyLineBeforeIndent = - lastNodeBeforeIndent.type !== "CallExpression" && - lastNodeBeforeIndent.type !== "OptionalCallExpression" && + !isCallOrOptionalCallExpression(lastNodeBeforeIndent) && shouldInsertEmptyLineAfter(lastNodeBeforeIndent); const expanded = concat([ printGroup(groups[0]), - shouldMerge ? concat(groups.slice(1, 2).map(printGroup)) : "", + shouldMerge ? printGroup(groups[1]) : "", shouldHaveEmptyLineBeforeIndent ? hardline : "", printIndentedGroup(groups.slice(shouldMerge ? 2 : 1)), ]); @@ -374,31 +365,75 @@ function printMemberChain(path, options, print) { .map(({ node }) => node) .filter(isCallOrOptionalCallExpression); - // We don't want to print in one line if the chain has: - // * A comment. - // * Non-trivial arguments. - // * Any group but the last one has a hard line. - // If the last group is a function it's okay to inline if it fits. + function looksLikeFluentConfigurationPattern() { + if ( + isExpressionStatement && + callExpressions.length > 1 && + // Keep simple chains like this on one line: + // req.checkBody("name").notEmpty().optional(); + !( + callExpressions[0].arguments.length <= 1 && + callExpressions.slice(1).every((expr) => expr.arguments.length === 0) + ) + ) { + const allArgs = flat(callExpressions.map((expr) => expr.arguments)); + return allArgs.length > 0 && allArgs.every(isLiteralLikeValue); + } + return false; + } + + function callHasComplexArguments(expr, index) { + return ( + (index !== 0 && expr.arguments.length > 2) || + !expr.arguments.every((arg) => isSimpleCallArgument(arg, 0)) + ); + } + + /** + * If the last call's argument is a function, it's okay to inline if it fits and there is no other function arguments. + * + * This chain should be split: + * + * const mapped = scopes.filter(scope => scope.value !== '').map((scope, i) => { + * // multi line content + * }); + * + * This chain can be inlined: + * + * const mapped = scopes.filter(myFilter).map((scope, i) => { + * // multi line content + * }); + * + */ + function lastGroupWillBreakAndOtherCallsHaveComplexArguments() { + const lastGroupNode = getLast(getLast(groups)).node; + const lastGroupDoc = getLast(printedGroups); + return ( + isCallOrOptionalCallExpression(lastGroupNode) && + willBreak(lastGroupDoc) && + callExpressions.some( + (expr, index) => + index !== callExpressions.length - 1 && + callHasComplexArguments(expr, index) + ) + ); + } + + // We don't want to print in one line if at least one of these conditions occurs: + // * the chain has comments, + // * the head of the chain is a constructor call, + // * the chain is an expression statement and all the arguments are literal-like ("fluent configuration" pattern), + // * the chain is longer than 2 calls and has non-trivial arguments or more than 2 arguments in any call but the first one, + // * any group but the last one has a hard line, + // * the last call's arguments have a hard line and other calls have non-trivial arguments. if ( hasComment || + printedNodes[0].node.type === "NewExpression" || + looksLikeFluentConfigurationPattern() || (callExpressions.length > 2 && - callExpressions.some( - (expr) => !expr.arguments.every((arg) => isSimpleCallArgument(arg, 0)) - )) || + callExpressions.some(callHasComplexArguments)) || printedGroups.slice(0, -1).some(willBreak) || - /** - * scopes.filter(scope => scope.value !== '').map((scope, i) => { - * // multi line content - * }) - */ - (((lastGroupDoc, lastGroupNode) => - isCallOrOptionalCallExpression(lastGroupNode) && willBreak(lastGroupDoc))( - getLast(printedGroups), - getLast(getLast(groups)).node - ) && - callExpressions - .slice(0, -1) - .some((n) => n.arguments.some(isFunctionOrArrowExpression))) + lastGroupWillBreakAndOtherCallsHaveComplexArguments() ) { return group(expanded); } diff --git a/src/language-js/utils.js b/src/language-js/utils.js index 1f7046765..e827bdf46 100644 --- a/src/language-js/utils.js +++ b/src/language-js/utils.js @@ -145,6 +145,7 @@ function isLiteral(node) { node.type === "Literal" || node.type === "NullLiteral" || node.type === "NumericLiteral" || + node.type === "BigIntLiteral" || node.type === "RegExpLiteral" || node.type === "StringLiteral" || node.type === "TemplateLiteral" || @@ -153,6 +154,24 @@ function isLiteral(node) { ); } +function isLiteralLikeValue(node) { + return ( + isLiteral(node) || + (node.type === "Identifier" && /^[A-Z_]+$/.test(node.name)) || + (node.type === "ArrayExpression" && + node.elements.every( + (element) => element !== null && isLiteralLikeValue(element) + )) || + (node.type === "ObjectExpression" && + node.properties.every( + (property) => + !property.computed && + property.value && + isLiteralLikeValue(property.value) + )) + ); +} + function isNumericLiteral(node) { return ( node.type === "NumericLiteral" || @@ -918,10 +937,12 @@ function isLongCurriedCallExpression(path) { * @returns {boolean} */ function isSimpleCallArgument(node, depth) { - if (depth >= 2) { + if (depth >= 3) { return false; } - const isChildSimple = (child) => isSimpleCallArgument(child, depth + 1); + + const plusOne = (node) => isSimpleCallArgument(node, depth + 1); + const plusTwo = (node) => isSimpleCallArgument(node, depth + 2); const regexpPattern = (node.type === "Literal" && node.regex && node.regex.pattern) || @@ -933,60 +954,62 @@ function isSimpleCallArgument(node, depth) { if ( node.type === "Literal" || + node.type === "BigIntLiteral" || node.type === "BooleanLiteral" || node.type === "NullLiteral" || node.type === "NumericLiteral" || + node.type === "RegExpLiteral" || node.type === "StringLiteral" || node.type === "Identifier" || node.type === "ThisExpression" || node.type === "Super" || - node.type === "BigIntLiteral" || node.type === "PrivateName" || node.type === "ArgumentPlaceholder" || - node.type === "RegExpLiteral" || node.type === "Import" ) { return true; } + if (node.type === "TemplateLiteral") { - return node.expressions.every(isChildSimple); + return node.expressions.every(plusTwo); } + if (node.type === "ObjectExpression") { return node.properties.every( - (p) => !p.computed && (p.shorthand || (p.value && isChildSimple(p.value))) + (p) => !p.computed && (p.shorthand || (p.value && plusTwo(p.value))) ); } + if (node.type === "ArrayExpression") { - return node.elements.every((x) => x == null || isChildSimple(x)); + return node.elements.every((x) => x === null || plusTwo(x)); } + if ( node.type === "CallExpression" || node.type === "OptionalCallExpression" || node.type === "NewExpression" ) { - return ( - isSimpleCallArgument(node.callee, depth) && - node.arguments.every(isChildSimple) - ); + return plusOne(node.callee, depth) && node.arguments.every(plusTwo); } + if ( node.type === "MemberExpression" || node.type === "OptionalMemberExpression" ) { - return ( - isSimpleCallArgument(node.object, depth) && - isSimpleCallArgument(node.property, depth) - ); + return plusOne(node.object, depth) && plusOne(node.property, depth); } + if ( node.type === "UnaryExpression" && (node.operator === "!" || node.operator === "-") ) { - return isSimpleCallArgument(node.argument, depth); + return plusOne(node.argument, depth); } + if (node.type === "TSNonNullExpression") { - return isSimpleCallArgument(node.expression, depth); + return plusOne(node.expression, depth); } + return false; } @@ -1056,6 +1079,7 @@ module.exports = { isJSXWhitespaceExpression, isLastStatement, isLiteral, + isLiteralLikeValue, isLongCurriedCallExpression, isSimpleCallArgument, isMeaningfulJSXText, diff --git a/tests/arrays/__snapshots__/jsfmt.spec.js.snap b/tests/arrays/__snapshots__/jsfmt.spec.js.snap index 01a33e0ea..16d256257 100644 --- a/tests/arrays/__snapshots__/jsfmt.spec.js.snap +++ b/tests/arrays/__snapshots__/jsfmt.spec.js.snap @@ -30,7 +30,10 @@ new Test() .test(); =====================================output===================================== -new Test().test().test([, 0]).test(); +new Test() + .test() + .test([, 0]) + .test(); ================================================================================ `; diff --git a/tests/flow/generators/__snapshots__/jsfmt.spec.js.snap b/tests/flow/generators/__snapshots__/jsfmt.spec.js.snap index ea64fd7f2..c3865f9a0 100644 --- a/tests/flow/generators/__snapshots__/jsfmt.spec.js.snap +++ b/tests/flow/generators/__snapshots__/jsfmt.spec.js.snap @@ -245,9 +245,15 @@ if (typeof infer_stmt_next === "undefined") { (infer_stmt_next: boolean); // error: string ~> boolean } -examples.widen_next().next(0); -examples.widen_next().next(""); -examples.widen_next().next(true); +examples + .widen_next() + .next(0); +examples + .widen_next() + .next(""); +examples + .widen_next() + .next(true); for (var x of examples.widen_yield()) { if (typeof x === "number") { @@ -257,13 +263,17 @@ for (var x of examples.widen_yield()) { } } -examples.delegate_next_generator().next(""); +examples + .delegate_next_generator() + .next(""); for (var x of examples.delegate_yield_generator()) { (x: number); // error: string ~> number } -examples.delegate_next_iterable([]).next(""); // error: Iterator has no next value +examples + .delegate_next_iterable([]) + .next(""); // error: Iterator has no next value for (var x of examples.delegate_yield_iterable([])) { (x: string); // error: number ~> string diff --git a/tests/method-chain/__snapshots__/jsfmt.spec.js.snap b/tests/method-chain/__snapshots__/jsfmt.spec.js.snap index 078c2461c..bf4897746 100644 --- a/tests/method-chain/__snapshots__/jsfmt.spec.js.snap +++ b/tests/method-chain/__snapshots__/jsfmt.spec.js.snap @@ -400,6 +400,28 @@ angular ================================================================================ `; +exports[`complex-args.js format 1`] = ` +====================================options===================================== +parsers: ["flow", "typescript"] +printWidth: 80 + | printWidth +=====================================input====================================== +client.execute( + Post.selectAll() + .where(Post.id.eq(42)) + .where(Post.published.eq(true)) +); + +=====================================output===================================== +client.execute( + Post.selectAll() + .where(Post.id.eq(42)) + .where(Post.published.eq(true)) +); + +================================================================================ +`; + exports[`computed.js format 1`] = ` ====================================options===================================== parsers: ["flow", "typescript"] @@ -533,6 +555,32 @@ object[ ================================================================================ `; +exports[`cypress.js format 1`] = ` +====================================options===================================== +parsers: ["flow", "typescript"] +printWidth: 80 + | printWidth +=====================================input====================================== +cy.get('option:first') + .should('be.selected') + .and('have.value', 'Metallica') + +cy.get(".ready") + .should("have.text", "FOO") + .should("have.css", "color", "#aaa"); + +=====================================output===================================== +cy.get("option:first") + .should("be.selected") + .and("have.value", "Metallica"); + +cy.get(".ready") + .should("have.text", "FOO") + .should("have.css", "color", "#aaa"); + +================================================================================ +`; + exports[`d3.js format 1`] = ` ====================================options===================================== parsers: ["flow", "typescript"] @@ -544,7 +592,7 @@ d3.select('body') .at({ width: 30, fill: '#f0f' }) .st({ fontWeight: 600 }) -d3.scaleLinear() +const myScale = d3.scaleLinear() .domain([1950, 1980]) .range([0, width]) @@ -563,7 +611,7 @@ d3.select("body") .at({ width: 30, fill: "#f0f" }) .st({ fontWeight: 600 }); -d3.scaleLinear().domain([1950, 1980]).range([0, width]); +const myScale = d3.scaleLinear().domain([1950, 1980]).range([0, width]); not.d3 .select("body") @@ -655,6 +703,54 @@ function f() { ================================================================================ `; +exports[`fluent-configuration.js format 1`] = ` +====================================options===================================== +parsers: ["flow", "typescript"] +printWidth: 80 + | printWidth +=====================================input====================================== +domain + .concept('Page') + .val('title', 'string') + .vals('widgets', 'Widget') +domain + .concept('Widget') + .val('title', 'string') + .val('color', 'Color') + .val('foo', 'Foo') + .val('bar', 'Bar') +domain + .concept('Widget') + .val('title', 'string') + .val('color', 'Color') +domain + .concept(CONCEPT_NAME) + .val('title') + .vals() + +=====================================output===================================== +domain + .concept("Page") + .val("title", "string") + .vals("widgets", "Widget"); +domain + .concept("Widget") + .val("title", "string") + .val("color", "Color") + .val("foo", "Foo") + .val("bar", "Bar"); +domain + .concept("Widget") + .val("title", "string") + .val("color", "Color"); +domain + .concept(CONCEPT_NAME) + .val("title") + .vals(); + +================================================================================ +`; + exports[`inline_merge.js format 1`] = ` ====================================options===================================== parsers: ["flow", "typescript"] @@ -700,6 +796,68 @@ var jqxhr = $.ajax("example.php").done(doneFn).fail(failFn); ================================================================================ `; +exports[`issue-3594.js format 1`] = ` +====================================options===================================== +parsers: ["flow", "typescript"] +printWidth: 80 + | printWidth +=====================================input====================================== +const fetched = fetch("/foo"); +fetched + .then(response => response.json()) + .then(json => processThings(json.data.things)); + +let column = new Column(null, conn) + .table(data.table) + .json(data.column); + +=====================================output===================================== +const fetched = fetch("/foo"); +fetched + .then((response) => response.json()) + .then((json) => processThings(json.data.things)); + +let column = new Column(null, conn) + .table(data.table) + .json(data.column); + +================================================================================ +`; + +exports[`issue-3621.js format 1`] = ` +====================================options===================================== +parsers: ["flow", "typescript"] +printWidth: 80 + | printWidth +=====================================input====================================== +const palindrome = str => { + const s = str.toLowerCase().replace(/[\\W_]/g, ''); + return s === s.split('').reverse().join(''); +}; + +const apiCurrencies = api().currencies().all() + +expect(cells.at(1).render().text()).toBe('link text1') +expect(cells.at(2).render().text()).toBe('link text2') +expect(cells.at(3).render().text()).toBe('link text3') +expect(cells.at(4).render().text()).toBe('link text4') + +=====================================output===================================== +const palindrome = (str) => { + const s = str.toLowerCase().replace(/[\\W_]/g, ""); + return s === s.split("").reverse().join(""); +}; + +const apiCurrencies = api().currencies().all(); + +expect(cells.at(1).render().text()).toBe("link text1"); +expect(cells.at(2).render().text()).toBe("link text2"); +expect(cells.at(3).render().text()).toBe("link text3"); +expect(cells.at(4).render().text()).toBe("link text4"); + +================================================================================ +`; + exports[`issue-4125.js format 1`] = ` ====================================options===================================== parsers: ["flow", "typescript"] @@ -716,8 +874,8 @@ req.checkBody('name').notEmpty().optional(); const x = moment().add(1, 'day').valueOf() // should stay on one line: -obj.foo(1).foo(2).foo(3); -obj.foo(-1).foo(import('2')).foo(!x).check(/[A-Z]/); +const y = obj.foo(1).foo(2).foo(3); +const z = obj.foo(-1).foo(import('2')).foo(!x).check(/[A-Z]/); // better on multiple lines: somePromise.then(format).then((val)=>doSomething(val)).catch((err)=>handleError(err)) @@ -771,7 +929,7 @@ db.branch( sandbox.stub(config, 'get').withArgs('env').returns('dev') -moment.utc(userInput).hour(0).minute(0).second(0) +const date = moment.utc(userInput).hour(0).minute(0).second(0) fetchUser(id) .then(fetchAccountForUser) @@ -833,13 +991,13 @@ it('gets triggered by mouseenter', () => { wrapper.dive().find(Button).prop(); }); -x.a(true).b(null).c(123) -x.d('').e(\`\`).f(g) -x.d('').e(\`\${123}\`).f(g) -x.h(i.j).k(l()).m([n, o]) +const a1 = x.a(true).b(null).c(123) +const a2 = x.d('').e(\`\`).f(g) +const a3 = x.d('').e(\`\${123}\`).f(g) +const a4 = x.h(i.j).k(l()).m([n, o]) class X { y() { - x.a(this).b(super.cde()).f(/g/).h(new i()).j() + const j = x.a(this).b(super.cde()).f(/g/).h(new i()).j(); } } @@ -867,8 +1025,8 @@ req.checkBody("name").notEmpty().optional(); const x = moment().add(1, "day").valueOf(); // should stay on one line: -obj.foo(1).foo(2).foo(3); -obj.foo(-1).foo(import("2")).foo(!x).check(/[A-Z]/); +const y = obj.foo(1).foo(2).foo(3); +const z = obj.foo(-1).foo(import("2")).foo(!x).check(/[A-Z]/); // better on multiple lines: somePromise @@ -916,7 +1074,11 @@ Object.keys(props) return a; }, {}); -point().x(4).y(3).z(6).plot(); +point() + .x(4) + .y(3) + .z(6) + .plot(); assert.equal(this.$().text().trim(), "1000"); @@ -933,7 +1095,7 @@ db.branch( sandbox.stub(config, "get").withArgs("env").returns("dev"); -moment.utc(userInput).hour(0).minute(0).second(0); +const date = moment.utc(userInput).hour(0).minute(0).second(0); fetchUser(id).then(fetchAccountForUser).catch(handleFetchError); @@ -994,13 +1156,13 @@ it("gets triggered by mouseenter", () => { wrapper.dive().find(Button).prop(); }); -x.a(true).b(null).c(123); -x.d("").e(\`\`).f(g); -x.d("").e(\`\${123}\`).f(g); -x.h(i.j).k(l()).m([n, o]); +const a1 = x.a(true).b(null).c(123); +const a2 = x.d("").e(\`\`).f(g); +const a3 = x.d("").e(\`\${123}\`).f(g); +const a4 = x.h(i.j).k(l()).m([n, o]); class X { y() { - x.a(this).b(super.cde()).f(/g/).h(new i()).j(); + const j = x.a(this).b(super.cde()).f(/g/).h(new i()).j(); } } @@ -1218,6 +1380,42 @@ of("test") ================================================================================ `; +exports[`pr-7889.js format 1`] = ` +====================================options===================================== +parsers: ["flow", "typescript"] +printWidth: 80 + | printWidth +=====================================input====================================== +const Profile = view.with({ name: (state) => state.name }).as((props) => ( +