Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Break long member and method chain #15171

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
+167 −61
Open

Break long member and method chain #15171

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
7770728
add as-is test
seiyab Jul 29, 2023
9920e6a
fix #2548
seiyab Jul 29, 2023
7c56d8e
update snapshots
seiyab Jul 29, 2023
6065709
add test
seiyab Jul 29, 2023
946c90e
refactor
seiyab Jul 29, 2023
6703814
delete a file I mistakenly commited
seiyab Jul 29, 2023
69ee580
write changelog
seiyab Jul 29, 2023
104e407
don't pass objectDoc in method chain
seiyab Jul 29, 2023
c6c9d6b
revert member-chain.js
seiyab Jul 29, 2023
727efa7
change coding style into probably preferred one in this project
seiyab Jul 29, 2023
10ef048
add test
seiyab Jul 30, 2023
2073238
avoid (probably) unrelated issue
seiyab Jul 30, 2023
fcd3d66
avoid indent for member in tail groups
seiyab Aug 31, 2023
ddcf1ca
change test cases
seiyab Sep 3, 2023
f31596c
fix some bugs around linebreak / indent
seiyab Sep 14, 2023
b30098e
update snapshots
seiyab Sep 14, 2023
21fe696
fix a bug relates to inlining
seiyab Sep 16, 2023
6741ed8
remove an unnecessary line
seiyab Sep 16, 2023
4c5265c
resolve idempotency problem
seiyab Sep 18, 2023
3bb92d4
Merge branch 'main' into 2548-long-long-member-and-method-chain
seiyab Sep 21, 2023
78ac561
remove useless line
seiyab Oct 18, 2023
99d389d
Merge branch 'main' into 2548-long-long-member-and-method-chain
seiyab Oct 18, 2023
20441e0
Merge branch 'main' into 2548-long-long-member-and-method-chain
seiyab Dec 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions changelog_unreleased/javascript/15171.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#### Break long member and method chain (#15171 by @seiyab)

<!-- prettier-ignore -->
```js
// Input
this.is.a.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.method();

// Prettier stable
this.is.a.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.method();


// Prettier main
this.is.a.long.long.long.long.long.long.long.long.long.long.long.long.long.long
.long.long.method();

```
42 changes: 35 additions & 7 deletions src/language-js/print/member-chain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,11 @@ import {
conditionalGroup,
breakParent,
label,
softline,
} from "../../document/builders.js";
import { willBreak } from "../../document/utils.js";
import printCallArguments from "./call-arguments.js";
import { printMemberLookup } from "./member.js";
import { printMemberLookup, shouldInlineMember } from "./member.js";
import {
printOptionalToken,
printFunctionTypeParameters,
Expand Down Expand Up @@ -106,7 +107,9 @@ function printMemberChain(path, options, print) {
} else if (isMemberish(node)) {
printedNodes.unshift({
node,
path,
fisker marked this conversation as resolved.
Show resolved Hide resolved
needsParens: pathNeedsParens(path, options),
shouldInline: isMemberExpression(node) && shouldInlineMember(path),
printed: printComments(
path,
isMemberExpression(node)
Expand Down Expand Up @@ -295,8 +298,29 @@ function printMemberChain(path, options, print) {
!hasComment(groups[1][0].node) &&
shouldNotWrap(groups);

function printGroup(printedGroup) {
const printed = printedGroup.map((tuple) => tuple.printed);
function printGroup(
printedGroup,
/** @type {{isFirstGroup?: boolean, isOneLine?: boolean}} */
opts = {},
) {
const { isFirstGroup = false, isOneLine = false } = opts;
const printed = printedGroup.map((printedNode, i) => {
if (
!isMemberish(printedNode.node) ||
(!isOneLine && i === 0) ||
printedNode.shouldInline
) {
return printedNode.printed;
}
const shouldBreak =
i !== 0 &&
hasComment(printedGroup[i - 1].node, CommentCheckFlags.Trailing);
const maybeBreakParent = shouldBreak ? breakParent : "";
if (isFirstGroup === true || isOneLine) {
return group(indent([maybeBreakParent, softline, printedNode.printed]));
}
return group([maybeBreakParent, softline, printedNode.printed]);
});
// Checks if the last node (i.e. the parent node) needs parens and print
// accordingly
if (printedGroup.length > 0 && printedGroup.at(-1).needsParens) {
Expand All @@ -313,8 +337,12 @@ function printMemberChain(path, options, print) {
return indent(group([hardline, join(hardline, groups.map(printGroup))]));
}

const printedGroups = groups.map(printGroup);
const oneLine = printedGroups;
const printedGroups = groups.map((doc, i) =>
printGroup(doc, { isFirstGroup: i === 0 }),
);
const oneLine = groups.map((doc, i) =>
printGroup(doc, { isFirstGroup: i === 0, isOneLine: true }),
);

const cutoff = shouldMerge ? 3 : 2;
const flatGroups = groups.flat();
Expand Down Expand Up @@ -346,8 +374,8 @@ function printMemberChain(path, options, print) {
shouldInsertEmptyLineAfter(lastNodeBeforeIndent);

const expanded = [
printGroup(groups[0]),
shouldMerge ? groups.slice(1, 2).map(printGroup) : "",
printGroup(groups[0], { isFirstGroup: true }),
shouldMerge ? groups.slice(1, 2).map((doc) => printGroup(doc)) : "",
shouldHaveEmptyLineBeforeIndent ? hardline : "",
printIndentedGroup(groups.slice(shouldMerge ? 2 : 1)),
];
Expand Down
67 changes: 43 additions & 24 deletions src/language-js/print/member.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,55 @@ import {
} from "../utils/index.js";
import { printOptionalToken } from "./misc.js";

/**
* @typedef {import("../../common/ast-path.js").default} AstPath
* @typedef {import("../../document/builders.js").Doc} Doc
*/

function printMemberExpression(path, options, print) {
const objectDoc = print("object");
const lookupDoc = printMemberLookup(path, options, print);
return label(objectDoc.label, [
objectDoc,
shouldInlineMember(path, objectDoc)
? lookupDoc
: group(indent([softline, lookupDoc])),
]);
}

/**
* @param {AstPath} path
* @param {*} options
* @param {*} print
* @returns {Doc}
*/
function printMemberLookup(path, options, print) {
const property = print("property");
const { node } = path;
const optional = printOptionalToken(path);

if (!node.computed) {
return [optional, ".", property];
}
if (!node.property || isNumericLiteral(node.property)) {
return [optional, "[", property, "]"];
}
return group([optional, "[", indent([softline, property]), softline, "]"]);
}

/**
* @param {AstPath} path
* @param {*} [objectDoc]
* @returns {boolean}
*/
function shouldInlineMember(path, objectDoc) {
const { node, parent } = path;
const firstNonMemberParent = path.findAncestor(
(node) =>
!(isMemberExpression(node) || node.type === "TSNonNullExpression"),
);

const shouldInline =
return (
(firstNonMemberParent &&
(firstNonMemberParent.type === "NewExpression" ||
firstNonMemberParent.type === "BindExpression" ||
Expand All @@ -31,28 +70,8 @@ function printMemberExpression(path, options, print) {
(node.object.type === "TSNonNullExpression" &&
isCallExpression(node.object.expression) &&
node.object.expression.arguments.length > 0) ||
objectDoc.label?.memberChain));

return label(objectDoc.label, [
objectDoc,
shouldInline ? lookupDoc : group(indent([softline, lookupDoc])),
]);
}

function printMemberLookup(path, options, print) {
const property = print("property");
const { node } = path;
const optional = printOptionalToken(path);

if (!node.computed) {
return [optional, ".", property];
}

if (!node.property || isNumericLiteral(node.property)) {
return [optional, "[", property, "]"];
}

return group([optional, "[", indent([softline, property]), softline, "]"]);
objectDoc?.label?.memberChain))
);
}

export { printMemberExpression, printMemberLookup };
export { printMemberExpression, printMemberLookup, shouldInlineMember };
45 changes: 18 additions & 27 deletions tests/format/js/arrow-call/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,26 +53,23 @@ const testResults = results.testResults.map((testResult) =>
);

it("mocks regexp instances", () => {
expect(() =>
moduleMocker.generateFromMetadata(moduleMocker.getMetadata(/a/)),
).not.toThrow();
expect(() => moduleMocker.generateFromMetadata(moduleMocker.getMetadata(/a/)))
.not.toThrow();
});

expect(() => asyncRequest({ url: "/test-endpoint" })).toThrowError(
/Required parameter/,
);

expect(() =>
asyncRequest({ url: "/test-endpoint-but-with-a-long-url" }),
).toThrowError(/Required parameter/);
expect(() => asyncRequest({ url: "/test-endpoint-but-with-a-long-url" }))
.toThrowError(/Required parameter/);

expect(() =>
asyncRequest({ url: "/test-endpoint-but-with-a-suuuuuuuuper-long-url" }),
).toThrowError(/Required parameter/);

expect(() =>
asyncRequest({ type: "foo", url: "/test-endpoint" }),
).not.toThrowError();
expect(() => asyncRequest({ type: "foo", url: "/test-endpoint" })).not
.toThrowError();

expect(() =>
asyncRequest({ type: "foo", url: "/test-endpoint-but-with-a-long-url" }),
Expand Down Expand Up @@ -154,26 +151,23 @@ const testResults = results.testResults.map((testResult) =>
);

it("mocks regexp instances", () => {
expect(() =>
moduleMocker.generateFromMetadata(moduleMocker.getMetadata(/a/)),
).not.toThrow();
expect(() => moduleMocker.generateFromMetadata(moduleMocker.getMetadata(/a/)))
.not.toThrow();
});

expect(() => asyncRequest({ url: "/test-endpoint" })).toThrowError(
/Required parameter/,
);

expect(() =>
asyncRequest({ url: "/test-endpoint-but-with-a-long-url" }),
).toThrowError(/Required parameter/);
expect(() => asyncRequest({ url: "/test-endpoint-but-with-a-long-url" }))
.toThrowError(/Required parameter/);

expect(() =>
asyncRequest({ url: "/test-endpoint-but-with-a-suuuuuuuuper-long-url" }),
).toThrowError(/Required parameter/);

expect(() =>
asyncRequest({ type: "foo", url: "/test-endpoint" }),
).not.toThrowError();
expect(() => asyncRequest({ type: "foo", url: "/test-endpoint" })).not
.toThrowError();

expect(() =>
asyncRequest({ type: "foo", url: "/test-endpoint-but-with-a-long-url" }),
Expand Down Expand Up @@ -255,26 +249,23 @@ const testResults = results.testResults.map((testResult) =>
);

it("mocks regexp instances", () => {
expect(() =>
moduleMocker.generateFromMetadata(moduleMocker.getMetadata(/a/))
).not.toThrow();
expect(() => moduleMocker.generateFromMetadata(moduleMocker.getMetadata(/a/)))
.not.toThrow();
});

expect(() => asyncRequest({ url: "/test-endpoint" })).toThrowError(
/Required parameter/
);

expect(() =>
asyncRequest({ url: "/test-endpoint-but-with-a-long-url" })
).toThrowError(/Required parameter/);
expect(() => asyncRequest({ url: "/test-endpoint-but-with-a-long-url" }))
.toThrowError(/Required parameter/);

expect(() =>
asyncRequest({ url: "/test-endpoint-but-with-a-suuuuuuuuper-long-url" })
).toThrowError(/Required parameter/);

expect(() =>
asyncRequest({ type: "foo", url: "/test-endpoint" })
).not.toThrowError();
expect(() => asyncRequest({ type: "foo", url: "/test-endpoint" })).not
.toThrowError();
Comment on lines -275 to +268
Copy link
Sponsor Contributor Author

@seiyab seiyab Jul 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to get worse to read.
Currently, I consider this to be another issue.

This comment was marked as outdated.

Sign in to view


expect(() =>
asyncRequest({ type: "foo", url: "/test-endpoint-but-with-a-long-url" })
Expand Down
5 changes: 2 additions & 3 deletions tests/format/js/break-calls/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,8 @@ const mapChargeItems = fp.flow(
(l) => Immutable.Range(l).toMap(),
);

expect(
new LongLongLongLongLongRange([0, 0], [0, 0]),
).toEqualAtomLongLongLongLongRange(new LongLongLongRange([0, 0], [0, 0]));
expect(new LongLongLongLongLongRange([0, 0], [0, 0]))
.toEqualAtomLongLongLongLongRange(new LongLongLongRange([0, 0], [0, 0]));

["red", "white", "blue", "black", "hotpink", "rebeccapurple"].reduce(
(allColors, color) => {
Expand Down
11 changes: 11 additions & 0 deletions tests/format/js/method-chain/2548-long-long-member-chain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
this.is.a.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.method();

this.is.a.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long()
.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.method.chain();

this.is.a.longlonglonglonglonglonglonglonglonglonglong["long long long long long method with bracket notation"]()
.and?.longlonglonglonglong?.longlonglonglonglong?.longlonglonglonglong?.longlonglonglonglong?.optional?.method?.()
.and.longlonglonglonglong.longlonglonglonglong.longlonglonglonglong.longlonglonglonglong.trailing.property.access;

this.is.a./* comment */ method /* comment */.chain // comment
.with.some.comments()
43 changes: 43 additions & 0 deletions tests/format/js/method-chain/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,48 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`2548-long-long-member-chain.js format 1`] = `
====================================options=====================================
parsers: ["babel", "flow", "typescript"]
printWidth: 80
| printWidth
=====================================input======================================
this.is.a.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.method();

this.is.a.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long()
.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long.method.chain();

this.is.a.longlonglonglonglonglonglonglonglonglonglong["long long long long long method with bracket notation"]()
.and?.longlonglonglonglong?.longlonglonglonglong?.longlonglonglonglong?.longlonglonglonglong?.optional?.method?.()
.and.longlonglonglonglong.longlonglonglonglong.longlonglonglonglong.longlonglonglonglong.trailing.property.access;

this.is.a./* comment */ method /* comment */.chain // comment
.with.some.comments()

=====================================output=====================================
this.is.a.long.long.long.long.long.long.long.long.long.long.long.long.long.long
.long.long.method();

this.is.a.long.long.long.long.long.long.long.long.long.long.long.long.long.long
.long.long
.long()
.long.long.long.long.long.long.long.long.long.long.long.long.long.long.long
.long.long.long.method.chain();

this.is.a.longlonglonglonglonglonglonglonglonglonglong[
"long long long long long method with bracket notation"
]().and?.longlonglonglonglong?.longlonglonglonglong?.longlonglonglonglong
?.longlonglonglonglong?.optional?.method?.().and.longlonglonglonglong
.longlonglonglonglong.longlonglonglonglong.longlonglonglonglong.trailing
.property.access;

this.is.a./* comment */ method /* comment */
.chain // comment
.with.some
.comments();

================================================================================
`;

exports[`13018.js format 1`] = `
====================================options=====================================
parsers: ["babel", "flow", "typescript"]
Expand Down
Loading