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
Consider breaking on text in JSX #963
Comments
cc @rattrayalex In the meantime, you can get unblocked by writing const Abc = () => {
// prettier-ignore
return (
<div>
Please state your <b>name</b> and <b>occupation</b> for the board of
directors.
</div>
);
}; |
Ooh, this gets really tricky (both in subjectivity and implementation). My guess is the tldr answer will be "no" but I'll think about it... |
@vjeux as you can see in the example the This only happens when the user has a long line with intermingled text and element siblings, not when the user has already broken the text. |
I'd be really interested to know what would make this difficult to implement (I currently have almost no knowledge of the inner working of Prettier). If someone could point me in the rough direction of where the JSX wrapping is controlled in the code I'd be happy to take a look next week. |
First, we'd need to split jsxtext on all whitespace, not just returns, and
treat each word like its own element. That might not be awesome for either
perf or code complexity but I don't think it'd be too bad.
Then you'd still need a way to group siblings in JSX. Right now we do that
by line. But if one line gets too long, every element in that line is
bumped to its own line. That's what you experienced.
Instead, we'd need to only bump the elements that were at the end of the
line, and I actually don't think such a thing is really possible with the
way prettier does line breaks... Entire groups break at a time, and you
don't know how many elements fit in a line a priori, so you can't just
shove only the elements that *would* fit into a line into a single group.
You're welcome to try to hack at it of course:
https://github.com/prettier/prettier/blob/master/src/printer.js#L2532 is
the beginning of where jsx text gets processed.
|
Thanks @rattrayalex, that is a great overview! Good to know what the difficulties are. I'll have a play around next week, but I won't expect to much. It does sound like JSX text would need a very different form of breaking than Prettier currently handles, and I'd be wary of adding a whole lot of complexity to support it. Still, will be interesting to play around with the code base to see what I can come up with. At the very least I'll learn a lot! |
Take it from me, you sure will! 😉
|
I've had a play around with the code, and while that didn't get me very far I did find something interesting in Walder's paper that inspired Prettier (http://homepages.inf.ed.ac.uk/wadler/papers/prettier/prettier.pdf). On page 15 it includes an example of formatting XML where it wraps text to include as many words as it can on a line before breaking. I wonder if we could add support for this to Prettier? So that I could specify a I don't think I know enough about the code base to work out how to add this myself. I'm not quite clear how Walder's paper maps to the Prettier code. |
Interesting – nice find! Thanks for sharing it here. One question is whether prettier would aim to collapse text that has been unnecessarily broken up. Eg; <div>
One sentence here.
Another short sentence here.
</div> Personally I think preserving the user's line breaks would be nice. I'd rather not have them collapse into, eg; <div>
One sentence here. Another short sentence here.
</div> Or, even worse, <div>One sentence here. Another short sentence here.</div> Which technically fits on the line, but isn't as readable or clear. If that's not a part of the feature you're proposing, then there may not be too many times that it will prove useful; pretty much only when there is a long line of text in JSX that has tags interspersed. In those cases, it will break in a subjectively ugly way, as you experienced – but it's not too difficult for the user to fix. It definitely sounds like a win to have word-based, minimal line breaks for JSX Text – I'm just personally a little doubtful that the benefit will justify the complexity of introducing a new doc type (which is what I assume would be required). |
I totally understand the concerns around introducing a new formatting rule for this case. Though I think it would be worth investigating further. I worry that with the current way JSX is formatted that users could lose trust in Prettier. For example, if the user starts with: text =
<div>
Please state your <b>name</b> and <b>occupation</b> for the board directors
</div> And then fix up their grammar mistake (forgetting the word of before directors): text =
<div>
Please state your <b>name</b> and <b>occupation</b> for the board of directors
</div> When they run Prettier it will unexpectedly turn it into: text = (
<div>
Please state your
{" "}
<b>name</b>
{" "}
and
{" "}
<b>occupation</b>
{" "}
for the board of directors
</div>
); The user then has to clean this up and manually format the code themselves. It may not be obvious to users that they need to add a line break themselves to avoid this happening. I only discovered that was a solution when attempting to create a minimal test case for this bug. |
Hmm. Yeah, this definitely does suck as-is. @vjeux , is there a way to trigger a |
I've tried to do it for + but I couldn't get it working with the primitives we have in prettier right now :( |
K. Let's leave this issue open for now then? I don't intend to dig into it myself in the near-term but @karl you're welcome to give it another shot and I may try again in the future. |
I would love to dig into this myself (probably in a couple of weeks time) but I would probably need some guidance in how the ideas/functions in Walder's paper map to the current Prettier code before I'm able to do anything useful. Happy to have this left open, and I'll add a comment here if when I start work on it again. |
src/doc-builders.js has the builder functions to generate the "doc" intermediate representation. src/doc-printer.js is the faithful implementation of the paper |
From #1021 (comment): "To summarize the discussion in #963, it is indeed unfortunate, but will probably require the addition of a fairly fundamental primitive to the prettier internals (that allows breaking a single line without breaking the entire group). I don't personally feel motivated to tackle this in the near term, but would like to see it happen, and might take it on later." |
One band-aid that could make the output a little less bad would be to put the text = (
<div>
Please state your
{" "}
<b>name</b>
{" "}
and
{" "}
<b>occupation</b>
{" "}
for the board of directors
</div>
); would at least be text = (
<div>
Please state your
{" "}<b>name</b>
{" "}and
{" "}<b>occupation</b>
{" "}for the board of directors
</div>
); which I think is much less disruptive, and is probably pretty trivial to implement. But I'm not sure if that'd be an improvement in the general case. |
Thanks for this discussion which helps to get a better understanding of why such result is generated for now. While a solution generating a clean output seems difficult to implement, would it be possible to detect such situation and point the developer to these files? This situation may occurs for other edge cases in the future too. And having them detected (e.g: as warnings, or with a dedicated command option) while pointing to an online documentation reference could help. The pointed web page could explain solutions to improve the resulting formatting. |
I have an open PR (#1120) which adds a new I'm hopefully that we can use it to automatically fix the issue raised here, and it should be flexible enough to re-use for other code structures, such as |
You did it! |
Would it be possible to break on text inside JSX?
At the moment it seems like we only break around tags. This can cause some unusual formatting when including text with bold/italic tags. We use paragraphs of text with formatting in our Storybook documentation.
Input:
Output:
Expected:
https://prettier.github.io/prettier/#%7B%22content%22%3A%22const%20Boo%20%3D%20()%20%3D%3E%20%7B%5Cn%20%20return%20(%5Cn%20%20%20%20%3Cdiv%3E%5Cn%20%20%20%20%20%20Please%20state%20your%20%3Cb%3Ename%3C%2Fb%3E%20and%20%3Cb%3Eoccupation%3C%2Fb%3E%20for%20the%20board%20of%20directors.%5Cn%20%20%20%20%3C%2Fdiv%3E%5Cn%20%20)%3B%5Cn%7D%5Cn%5Cnconst%20Yay%20%3D%20()%20%3D%3E%20%7B%5Cn%20%20return%20(%5Cn%20%20%20%20%3Cdiv%3E%5Cn%20%20%20%20%20%20Please%20state%20your%20%3Cb%3Ename%3C%2Fb%3E%20and%20%3Cb%3Eoccupation%3C%2Fb%3E%20for%20the%20board%5Cn%20%20%20%20%20%20of%20directors.%5Cn%20%20%20%20%3C%2Fdiv%3E%5Cn%20%20)%3B%5Cn%7D%5Cn%22%2C%22options%22%3A%7B%22printWidth%22%3A80%2C%22tabWidth%22%3A2%2C%22singleQuote%22%3Afalse%2C%22trailingComma%22%3A%22none%22%2C%22bracketSpacing%22%3Atrue%2C%22jsxBracketSameLine%22%3Afalse%2C%22parser%22%3A%22babylon%22%2C%22doc%22%3Afalse%7D%7D
The text was updated successfully, but these errors were encountered: