Skip to content

Don't break simple template literals#5979

Merged
duailibe merged 7 commits intoprettier:masterfrom
jwbay:template-literal-expressions
May 8, 2019
Merged

Don't break simple template literals#5979
duailibe merged 7 commits intoprettier:masterfrom
jwbay:template-literal-expressions

Conversation

@jwbay
Copy link
Copy Markdown
Contributor

@jwbay jwbay commented Mar 16, 2019

A simple template literal is initially defined as a literal wherein all the expressions are identifiers or member access expressions where all parts are identifiers. We print these expressions into strings with infinite print width before printing the containing template string. Adding a function to determine whether a template is "simple" allows us to expand the heuristic in a central location over time.

This handles some of #3368, but I'm not sure it should be closed as there's a lot of discussion over heuristics there.

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
  • I’ve read the contributing guidelines.

Try the playground for this PR

Copy link
Copy Markdown
Collaborator

@duailibe duailibe left a comment

Choose a reason for hiding this comment

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

I like it! This is awesome, thank you so much!

Just a quick question, why did you decide to check if all expressions are simple, instead of checking each one separately?

@jwbay
Copy link
Copy Markdown
Contributor Author

jwbay commented Mar 19, 2019

Just a quick question, why did you decide to check if all expressions are simple, instead of checking each one separately?

If we're going to break at all, I think it's better to let the normal break rules apply. This avoids a situation like

`asdf asdf asdf ${will.not.break} asdf asdf asdf ${will.not.break} asdf asdf asdf ${will.not.break} asdf ${some.complex.thing({ x: 42 }, y, z)} asdf`

printing like this (what do these arguments belong to? time to scroll)

`asdf asdf asdf ${will.not.break} asdf asdf asdf ${will.not.break} asdf asdf asdf ${will.not.break} asdf ${some.complex.thing(
  { x: 42 },
  y,
  z
)} asdf`

instead of this

`asdf asdf asdf ${will.not.break} asdf asdf asdf ${
  will.not.break
} asdf asdf asdf ${will.not.break} asdf ${some.complex.thing(
  { x: 42 },
  y,
  z
)} asdf`;

@jwbay
Copy link
Copy Markdown
Contributor Author

jwbay commented Apr 13, 2019

Anything that could move this forward?

@alexander-akait
Copy link
Copy Markdown
Member

Need rebase

jwbay added 2 commits April 13, 2019 13:11
A simple template literal is initially defined as a literal wherein all the expressions are identifiers or member access expressions where all parts are identifiers. We print these expressions into strings with infinite print width before printing the template.
@jwbay jwbay force-pushed the template-literal-expressions branch from 27fdaae to 71acfbd Compare April 13, 2019 17:13
@jwbay
Copy link
Copy Markdown
Contributor Author

jwbay commented Apr 13, 2019

Rebased 👍

@donaldpipowitch
Copy link
Copy Markdown

@jwbay, thank you so much for this PR. This could make our code incredible more readable in several places. @duailibe do you still need to approve this or is the approval from @evilebottnawi enough? @jwbay can you solve the conflict? That would be so useful. ❤️ I hope this gets merged.

@duailibe duailibe merged commit 6cee80b into prettier:master May 8, 2019
@duailibe
Copy link
Copy Markdown
Collaborator

duailibe commented May 8, 2019

Thank you @jwbay.

And sorry this took a while, thanks for being patient.

@graue
Copy link
Copy Markdown

graue commented Jun 19, 2019

Is this meant to keep expressions on one line even when they're over the line width? If so, any way we can get the 1.17 behavior back? Upgrading to 1.18 has forced a lot of horizontal scrolling in the codebase I'm working on, and to me, it is a big readability downgrade.

Here's an example:

-      result += `; Screen space, Available: ${window.screen.availWidth} x ${
-        window.screen.availHeight
-      }`
+      result += `; Screen space, Available: ${window.screen.availWidth} x ${window.screen.availHeight}`

I guess the first version is "weird" in the sense that it's a little asymmetrical, but from a practical point of view, being able to see all the code at once is much more helpful than having the rest of the line be a mystery and having to scroll.

@j-f1
Copy link
Copy Markdown
Member

j-f1 commented Jun 21, 2019

Is this meant to keep expressions on one line even when they're over the line width?

I believe so. You could enable soft wrapping in your editor to make this easier to read, though.

@graue
Copy link
Copy Markdown

graue commented Jun 21, 2019

@j-f1, thanks for the response! For me, soft wrapping is also very bad for readability.

What is Prettier's philosophy on configuration? If I'm in the minority in wanting template strings to wrap, would it be reasonable to ask for an option to restore that behavior? Would a diff adding that configuration option be likely to be accepted?

I won't belabor the point if most people disagree, but to explain where I'm coming from, I thought the point of Prettier was to wrap lines for readability, so this seems to be going against the core reason for the package. It's even in the second sentence of the readme:

[Prettier] enforces a consistent style by parsing your code and re-printing it with its own rules that take the maximum line length into account, wrapping code when necessary.

That's why I'm surprised by this patch.

@duailibe
Copy link
Copy Markdown
Collaborator

@graue Yes this is the core heuristic to decide when to add new lines in the code but isn't the only one. We do break code in multiple lines even when it doesn't exceed print width, and prevent breaking the code even if it exceeds the print width.

This doesn't mean we're not open to discussions and improvements (as long as it doesn't include an option, as much as possible); but this one specifically was one of the most requested changes in the last months of Prettier.

@FrederickEngelhardt
Copy link
Copy Markdown

@duailibe would it be possible to have a configuration to enable/disable this feature?

@duailibe
Copy link
Copy Markdown
Collaborator

@FrederickEngelhardt I don't think so

@jLouzado
Copy link
Copy Markdown

Is this meant to keep expressions on one line even when they're over the line width?

I believe so. You could enable soft wrapping in your editor to make this easier to read, though.

I'm having a hard time understanding the rationale for this change (even after reading all the threads). This change is literally breaking readability if we now need to enable an IDE option to fix it.

@alexander-akait
Copy link
Copy Markdown
Member

@jLouzado Can you provide examples?

@j-f1
Copy link
Copy Markdown
Member

j-f1 commented Nov 7, 2019

@evilebottnawi
Prettier 1.18.2
Playground link

--parser babel

Input:

`abc def ghi ${
  jkl
} mno pqr stu ${
  vwx.yz
} lorem ipsum dolor ${
  sit.amet
} etc etc etc lots ${
  and.lots.and.lots
} of words here`

Output:

`abc def ghi ${jkl} mno pqr stu ${vwx.yz} lorem ipsum dolor ${sit.amet} etc etc etc lots ${and.lots.and.lots} of words here`;

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Feb 5, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 5, 2020
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.

8 participants