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

Hug JSX text after multi line tags #1671

Merged
merged 2 commits into from
May 23, 2017

Conversation

karl
Copy link
Collaborator

@karl karl commented May 23, 2017

A fix for #1581

This PR no longer allows a line break between tags and text in JSX. This fixes an issue that was blocking adoption at Facebook (#1581 (comment)) with little adverse effects for our standard formatting.

.
this collection and the list it contained on <b>
{humanDate(since)}
</b>.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This looks like an edge case with the current implementation. When we have tag that would just fit on a line and it is followed by text it causes this unusual formatting.

I imagine in this case we would be aiming for:

this collection and the list it contained on 
<b>
  {humanDate(since)}
</b>.

I haven't had a chance to investigate why this is happening yet.

Copy link
Member

Choose a reason for hiding this comment

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

This PR looks great to me. Do you feel this is a blocker? I don't. We could always improve things in another PR.

Btw, I this would be a pretty nice formatting:

this collection and the list it contained on 
{" "}<b>{humanDate(since)}</b>.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, I don't really care about this.

src/printer.js Outdated
// tags and text.
// Unfortunately Facebook have a custom translation pipeline
// (https://github.com/prettier/prettier/issues/1581#issuecomment-300975032)
// that uses the JSX syntax, but does not follow the JSX whitespace
Copy link
Member

Choose a reason for hiding this comment

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

I would call this "the React whitespace rules" since JSX does not seem to have whitespace rules.

@vjeux
Copy link
Contributor

vjeux commented May 23, 2017

Thanks so much for working on this!

Could you also ensure that it works before as well?

<div>Elements Elements Elements Elements Elements Elements Elements Elements (<span>{count}</span>) Elements Elements Elements</div>

currently outputs

<div>
  Elements Elements Elements Elements Elements Elements Elements Elements (
  <span>{count}</span>) Elements Elements Elements
</div>;

which would add a space between ( and count.

@karl karl force-pushed the jsx-hug-text-after-multiline branch from 008550e to c38ff4f Compare May 23, 2017 16:38
@karl karl force-pushed the jsx-hug-text-after-multiline branch from 252be5b to 3872b24 Compare May 23, 2017 16:47
@karl
Copy link
Collaborator Author

karl commented May 23, 2017

I've pushed a quick change which hugs text to the beginning of tags as well.

Although there are some degenerative cases where you can end up with a line that never gets split.

Input:

this_really_should_split_across_lines =
  <div>
    before{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after
  </div>

Output:

this_really_should_split_across_lines = (
  <div>
    before{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after
  </div>
);

I'll investigate what we might be able to do to fix this.

@vjeux
Copy link
Contributor

vjeux commented May 23, 2017

Do people write this in practice?

@lydell
Copy link
Member

lydell commented May 23, 2017

@karl There's no way to split your last example, is it? Even if there is, I agree with @vjeux that it looks like a case where it's not worth spending time.

@karl
Copy link
Collaborator Author

karl commented May 23, 2017

Yep, totally agree that the degenerative case shouldn't block merging this!

@vjeux
Copy link
Contributor

vjeux commented May 23, 2017

@lydell mind reviewing this PR?

@karl
Copy link
Collaborator Author

karl commented May 23, 2017

@lydell I believe it should be theoretically possible to split it by doing something like:

  <div>
    before{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{
      stuff
    }after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{
      stuff
    }after{stuff}after{stuff}after
  </div>

@vjeux vjeux merged commit 2512497 into prettier:master May 23, 2017
@vjeux
Copy link
Contributor

vjeux commented May 24, 2017

Just found a case where it's not stable the first time if you are interested. I don't think it's a big deal and it's okay if it's not fixed, but you may be interested.

<div className="yourScore">
  Your score: <span className="score">{`${mini.crosstable.users[sessionUserId]} - ${mini.crosstable.users[user.id]}`}</span>
</div>
<div className="yourScore">
  Your score:
  {" "}
  <span className="score">{`${mini.crosstable.users[sessionUserId]} - ${mini
    .crosstable.users[user.id]}`}</span>
</div>;
<div className="yourScore">
  Your score:
  {" "}
  <span className="score">{`${mini.crosstable.users[sessionUserId]} - ${mini.crosstable.users[user.id]}`}</span>
</div>;

@karl karl mentioned this pull request May 24, 2017
@karl
Copy link
Collaborator Author

karl commented May 24, 2017

@vjeux I've created an issue for the unstable formatting and added it to my TODO list.

@karl karl deleted the jsx-hug-text-after-multiline branch June 26, 2017 19:43
@lydell lydell mentioned this pull request Apr 12, 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 20, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 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

3 participants