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

Last argument expansion works for arrow functions that return JSX #211

Merged
merged 2 commits into from Jan 15, 2017
+91 −2
Merged

Last argument expansion works for arrow functions that return JSX #211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d5c2e1
Last argument expansion works for arrow functions that return JSX
vjeux Jan 15, 2017
96e42df
Fixes #130
vjeux Jan 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/printer.js
View file
Open in desktop
Expand Up @@ -356,7 +356,11 @@ function genericPrintNoParens(path, options, print) {
const body = path.call(print, "body");
const collapsed = concat([ concat(parts), " ", body ]);

if (n.body.type === "JSXElement") {
if (
n.body.type === 'ArrayExpression' ||
n.body.type === 'ObjectExpression' ||
n.body.type === 'JSXElement'
) {
return group(collapsed);
}

Expand Down Expand Up @@ -1720,7 +1724,10 @@ function printArgumentsList(path, options, print) {
lastArg.type === "FunctionExpression" ||
lastArg.type === "ArrowFunctionExpression" &&
(lastArg.body.type === "BlockStatement" ||
lastArg.body.type === "ArrowFunctionExpression") ||
lastArg.body.type === "ArrowFunctionExpression" ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason why we have to enumerate all the types of body we want to support? Could I just get rid of the check on body.type?

Copy link
Member

Choose a reason for hiding this comment

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

I worked through this on my livestream, and had the exact same question! I removed the body.type check and saw why from the tests. I can't remember exactly why, but try it out.

I think the part of the reason is #61 actually. We allowed block-less arrow functions to break on the content, so if you have x => this.foo().bar(), it could actually break right after the arrow. That would look weird though, or even totally broken if there is more complex content, if we allow this last arg grouping behavior. So we only break blocks.

However, turns out of course there are other things we want to allow to break: objects, arrays, etc. But not everything. Feel free to think about this and let me know if there's a better solution! I'm fine if there's a different way that works for everything.

lastArg.body.type === "ObjectExpression" ||
lastArg.body.type === "ArrayExpression" ||
lastArg.body.type === "JSXElement") ||
lastArg.type === "NewExpression";

if (groupLastArg) {
Expand Down
57 changes: 57 additions & 0 deletions tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
@@ -0,0 +1,57 @@
exports[`test jsx.js 1`] = `
"const els = items.map(item => (
<div className=\"whatever\">
<span>{children}</span>
</div>
));
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
const els = items.map(item => (
<div className=\"whatever\">
<span>{children}</span>
</div>
));
"
`;

exports[`test object.js 1`] = `
"const formatData = pipe(
zip,
map(([ ref, data ]) => ({
nodeId: ref.nodeId.toString(),
...attributeFromDataValue(ref.attributeId, data)
})),
groupBy(prop(\'nodeId\')),
map(mergeAll),
values
);

export const setProp = y => ({
...y,
a: \'very, very, very long very, very long text\'
});

export const log = y => {
console.log(\'very, very, very long very, very long text\')
};
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
const formatData = pipe(
zip,
map(([ ref, data ]) => ({
nodeId: ref.nodeId.toString(),
...attributeFromDataValue(ref.attributeId, data)
})),
groupBy(prop(\"nodeId\")),
map(mergeAll),
values
);

export const setProp = y => ({
...y,
a: \"very, very, very long very, very long text\"
});

export const log = y => {
console.log(\"very, very, very long very, very long text\");
};
"
`;
1 change: 1 addition & 0 deletions tests/last_argument_expansion/jsfmt.spec.js
View file
Open in desktop
@@ -0,0 +1 @@
run_spec(__dirname);
5 changes: 5 additions & 0 deletions tests/last_argument_expansion/jsx.js
View file
Open in desktop
@@ -0,0 +1,5 @@
const els = items.map(item => (
<div className="whatever">
<span>{children}</span>
</div>
));
19 changes: 19 additions & 0 deletions tests/last_argument_expansion/object.js
View file
Open in desktop
@@ -0,0 +1,19 @@
const formatData = pipe(
zip,
map(([ ref, data ]) => ({
nodeId: ref.nodeId.toString(),
...attributeFromDataValue(ref.attributeId, data)
})),
groupBy(prop('nodeId')),
map(mergeAll),
values
);

export const setProp = y => ({
...y,
a: 'very, very, very long very, very long text'
});

export const log = y => {
console.log('very, very, very long very, very long text')
};