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

Prevent JSX from breaking a logical expression chain #5092

Merged
merged 4 commits into from Oct 10, 2018

Conversation

duailibe
Copy link
Member

@duailibe duailibe commented Sep 13, 2018

Closes #5089

Maybe we want to think about doing the same for objects and arrays

@@ -823,8 +823,7 @@ exports[`logical-expression.js - flow-verify 1`] = `
</div>;

<div>
{a &&
b && (
{a && b && (
<span>
Copy link
Member

Choose a reason for hiding this comment

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

This indentation level looks too deep

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, I missed that

@duailibe
Copy link
Member Author

duailibe commented Sep 21, 2018

I don't think there's a way to solve this without a new primitive, or using different (but worse IMO) heuristics..

What I'm currently doing is: if the last member is JSX, split in two groups: one for the JSX and another for everything else. In that case, we can either have (first uses indent and the other doesn't):

{a && b && (
    <Foo />
)}
{a &&
  b && (
    <Foo />
  )}

versus:

{a && b && (
  <Foo />
)}
{a &&
  b && (
  <Foo />
  )}

Copy link
Member

@suchipi suchipi left a comment

Choose a reason for hiding this comment

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

Requesting changes for clarification because the indentation level issue is a blocker

@suchipi
Copy link
Member

suchipi commented Sep 26, 2018

@duailibe I think we could solve it by checking if the last thing in the ternary chain is JSX when we print every ternary child, but it's duplicative work (from each child, walk up to the parent, then walk back down to check if the chain's last child is JSX). I guess we could use some kind of cache if we wanted to reduce the scope, but it'd be nice to have a primitive that works like React context for this particular use-case; something like (pseudocode):

  • when encountering ternary, walk down chain and check if last child is JSX
  • if it is, wrap the whole thing in a context("withinTernaryWithJSXAsLastChild", group(...))
  • when rendering each child, use something like ifContext("withinTernaryWithJSXAsLastChild", dontGroup(...), group(...))

@suchipi
Copy link
Member

suchipi commented Sep 26, 2018

The idea being that every doc node in the tree could use ifContext to ask for the current context and it would be the result of all context(...) nodes up the tree, eg with this structure:

context("foo",
  context("bar",
    group(...)
  )
)

Inside the group, context is { foo: true, bar: true }, so it can use ifContext("foo" to render one way or the other depending on if the context's foo property is true, etc.

@duailibe
Copy link
Member Author

duailibe commented Oct 8, 2018

@suchipi I was able to fix this using the groupId in ifBreak that @ikatyang created.

In that case, everything on the left of the chain is in a single group and I can indent or not depending if that group broke or not :-)

Copy link
Member

@suchipi suchipi left a comment

Choose a reason for hiding this comment

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

Woo!

@duailibe duailibe merged commit ed6a2d9 into prettier:master Oct 10, 2018
@duailibe duailibe deleted the binary-in-jsx branch October 10, 2018 17:11
@ikatyang ikatyang added this to the 1.15 milestone Oct 25, 2018
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 23, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 23, 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

7 participants