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.

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

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

Merged
merged 2 commits into from Jan 15, 2017

Conversation

vjeux
Copy link
Contributor

@vjeux vjeux commented Jan 15, 2017

Fixes #195
Fixes #130

@@ -1720,7 +1720,8 @@ 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.

@vjeux
Copy link
Contributor Author

vjeux commented Jan 15, 2017

I added a second commit on-top that fixes #130. It makes sure that not only JSXElement but also objects and arrays do not add another level of indentation.

@jlongster
Copy link
Member

Awesome! There might be a way to make this code simpler. Until then, this works.

@jlongster jlongster merged commit a4695b1 into prettier:master Jan 15, 2017
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 21, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants