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

Linebreaks weirdly in long ES6 template strings with expressions #3368

Closed
Swizec opened this issue Dec 1, 2017 · 100 comments
Closed

Linebreaks weirdly in long ES6 template strings with expressions #3368

Swizec opened this issue Dec 1, 2017 · 100 comments
Labels
area:template literals lang:javascript Issues affecting JS locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken

Comments

@Swizec
Copy link

Swizec commented Dec 1, 2017

Prettier 1.8.2
Playground link

# Options (if any):
--print-width=80

Input:

class Quote {
    character = {
        name: 'Dorian'
    }

    text = `Yes, ${this.character.name}, you will always be fond of me. I represent to you all the sins you have never had the courage to commit.`
}

Output:

class Quote {
  character = {
    name: "Dorian"
  };

  text = `Yes, ${
    this.character.name
  }, you will always be fond of me. I represent to you all the sins you have never had the courage to commit.`;
}

Expected behavior:

I would prefer a long string to not get split into multiple lines, even if it contains expressions. When there are no expressions, Prettier correctly lets a long string go beyond the print-width limit.

I can't decide if this is a bug or a personal preference, but it's been creating weird hard to read code in our codebase. We can clean most of it up by simplifying our expressions, but there's only so short we can go.

@ikatyang
Copy link
Member

ikatyang commented Dec 1, 2017

Ref: previous discussion #3315 (comment)

@ikatyang ikatyang added lang:javascript Issues affecting JS status:needs discussion Issues needing discussion and a decision to be made before action can be taken labels Dec 1, 2017
@duailibe
Copy link
Member

duailibe commented Dec 6, 2017

As we'll focus the discussion on this issue, I'll copy my comment from the other thread (cc @lipis):

The thing about template literals is without breaking between { and } we were actually printing:

const str1 = `text text text ${foo.
  bar} text text`;

const str2 = `text text text ${foo[
  bar
]} text text`;

which IMO are strictly worse than what we're printing right now. Note the foo.bar and foo[bar] are a group with "break points", meaning that if they don't fit in one line, they will break. We'd need to special case printing MemberExpressions when inside a template literal so this wouldn't happen.

The other problem with this is when should and shouldn't break in those cases:

foo = `Text text text text text ${foo.bar.baz.spam.eggs}`;
foo = `Text text text text text ${longFooBarBazSpam.longFooBarBazEggs}`;

Until we can find an heurisitic that's good enough to decide on those cases, it's better to keep consistent that always breaks.

@lipis
Copy link
Member

lipis commented Dec 6, 2017

I think those cases we shouldn't brake at all..

foo = `Text text text text text ${foo.bar.baz.spam.eggs}`;
foo = `Text text text text text ${longFooBarBazSpam.longFooBarBazEggs}`;

and consider that a single expression

while this one is not:

foo = `Text text text text text ${foo.bar.baz.spam.eggs + foo.bar.baz.bacon}`;
foo = `Text text text text text ${longFooBarBazSpam.longFooBarBazEggs - longFooBarBazSpam.longFooBarBazBacon}`;

@Swizec
Copy link
Author

Swizec commented Dec 6, 2017 via email

@lipis
Copy link
Member

lipis commented Dec 6, 2017

I would agree.. maybe it makes sense to not break them at all..

@duailibe
Copy link
Member

duailibe commented Dec 6, 2017

Discouraging messes is the job of a linter, not a formatter

The job of a formatter is to format the code, whether or not it's a mess, trying its best to output code that looks consistent. I don't think we should be in charge of encouraging or discouraging any pattern.

Don't get me wrong, I would love if people wrote code in specific styles.. that would make the job of a formatter a whole lot simpler, but that's not what we've noticed out there in codebases, both smaller and bigger ones.

As for the formatting issue, prettier used to not format template literals at all, but people complained code wasn't getting formatted. Then it started formatting but using removeLines to remove breaks which would output ${a.b.c.d.e} in a single line, but would break other more complex cases, with function calls and object expressions inside.

I see what we have today as a good compromise but I agree "simple" cases could get better. The whole point is deciding what is simple and have a sane way to format those cases without adding too much complexity to the code.

See #2046, #1893, #1662, #1626, #821, #1183 (and there's more :-)) for more context on this issue.

@jwbay
Copy link
Contributor

jwbay commented Dec 7, 2017

If there's a rule of thumb needed, I'd propose that inserting one existing value into a template string should never cause breaks. If you're doing something that could create a new value (getters aside), or picks between values, it's complex enough to warrant breaks.

Expressions that shouldn't break:

`${foo}` // this is done, which is awesome!
`${foo.bar}`
`${foo[bar]}`
`${foo[bar][baz]}`

There's some interesting middle ground that would be nice to have, but breaks with that rule of thumb:

`${foo()}` // all of the above as function calls with no arguments. i.e. just 'get this computed value'
`${foo || bar}` // to inline defaults without paying the 'break cost'. but with member access chains, this could get long. :shrug:

@danwfeeney
Copy link

danwfeeney commented Jan 4, 2018

I'd say a lot of people, including myself, could dig an option that reverts to previous behaviour i.e. rule to disable formatting template literals completely e.g. ignoreTemplateLiterals: true.

Does this exist already? Any reason this would be a problem?

@JonathanWolfe
Copy link

JonathanWolfe commented Jan 12, 2018

I definitely think that template literals should be treated as one holistic expressions and their contents should never be altered.

A travesty of formatting
log(
	chalk.bgRed(
		chalk.white(
			`Covered Lines below threshold: ${
				coverageSettings.lines
			}%. Actual: ${coverageSummary.total.lines.pct}%`
		)
	)
);

(Github displaying tabs as 8 spaces is also a travesty)

@j-f1
Copy link
Member

j-f1 commented Jan 12, 2018

@JonathanWolfe Would you agree that this should not be formatted?

`Covered Lines below threshold: ${
     coverageSettings
.lines

}%. Actual: ${  coverageSummary

.total.

lines
.
pct
  }%`

(that’s valid JS)

@JonathanWolfe
Copy link

JonathanWolfe commented Jan 12, 2018

@j-f1: Unfortunately, and it pains me to say this to that example, yes; I would leave that alone.

The copout answer is because nobody would write something like that, but really, I believe the contents of Template Strings/Literals/Expressions to be intentionally formatted by the author when they're written.

An ideal answer might be to leave the expression location alone, but format them as if it is a standalone line. This way the contents get formatted and made pretty, but the braces aren't broken apart.

Maybe an even simpler answer is to not allow line breaks when formatting template expressions.

@duailibe
Copy link
Member

@JonathanWolfe We used to not format and we changed it because a LOT of people expected the code to be formatted there as well. So it's not an option to stop formatting those expressions.

We do want to make the formatting better and we'll gladly take suggestions

@JonathanWolfe
Copy link

@duailibe: I get that expectation. I would say the best thing to do would be to not linebreak the braces of ${ } as it just makes the the template literal look awkward afterwards

@duailibe
Copy link
Member

Yeah that's an option.. If we simply remove the linebreaks, that would be formatted as:

const foo = `some text here ${foo
  .bar.baz} continues here`;

Which is worse than what we're doing right now. That's just how it prints a MemberExpression. So in that case we'd also need to special case if a MemberExpression is inside a TemplateLiteral, but what would mean for examples like:

const foo = `some text here ${foo.something({
  key: prop
}).bar.baz()}`;

Should it remain as is? That's inconsistent with how we print that case in other places:

foo
  .something({
    key: prop
  })
  .bar.baz();

Anyway, we're not opposed on improving that, it's just it's a difficult problem and we'd need a consistent suggestion of different examples and how we should print them. Hopefully that gives more context on the decisions on this subject.

@JonathanWolfe
Copy link

JonathanWolfe commented Jan 12, 2018

@duailibe: Sure, I've got lots of time right now so I'll make a big list of before/afters.

I went out and took a bunch of template literals from MDN, CSS-Tricks, and the lit-html repo.

It appears to me that the heuristic to use to get the best formatting is:

  • Format content inside template expressions as if they were any normal expression
  • Never add new lines, except:
    • When a function is present AND a parameter to that function is another function or another template literal
// Input
console.log(`Fifteen is ${a          + b} and not ${2*a    +
      
      b}.`);

// Current
console.log(`Fifteen is ${a + b} and not ${2 * a + b}.`);

// Desired - No change
console.log(`Fifteen is ${a + b} and not ${2 * a + b}.`); 
// Input
const foo = `some text here ${foo.something({
  key: prop
}).bar.baz()}`;

// Current
const foo = `some text here ${foo
  .something({
    key: prop,
  })
  .bar.baz()}`;

// Desired
const foo = `some text here ${foo.something({key: prop}).bar.baz()}`
// Input
const classes = `header ${ isLargeScreen() ? '' :
  (item.isCollapsed ? 'icon-expander' : 'icon-collapser') }`;
const classes = `header ${ isLargeScreen() ? '' :
 `icon-${(item.isCollapsed ? 'expander' : 'collapser')}` }`;

// Current
const classes = `header ${
  isLargeScreen() ? '' : item.isCollapsed ? 'icon-expander' : 'icon-collapser'
}`;
const classes = `header ${
  isLargeScreen() ? '' : `icon-${item.isCollapsed ? 'expander' : 'collapser'}`
}`;

// Desired
const classes = `header ${isLargeScreen() ? '' : item.isCollapsed ? 'icon-expander' : 'icon-collapser'}`;
const classes = `header ${isLargeScreen() ? '' : `icon-${item.isCollapsed ? 'expander' : 'collapser'}`}`;
// Input
var pronoun = person.parent.getPronoun({formal:true});
var output = myTag`that ${person.parent.getPronoun({ formal:true })} is a ${ person.parent.getAge() }`;

// Current
var pronoun = person.parent.getPronoun({ formal: true });
var output = myTag`that ${person.parent.getPronoun({
  formal: true,
})} is a ${person.parent.getAge()}`;

// Desired
var pronoun = person.parent.getPronoun({ formal: true });
var output = myTag`that ${person.parent.getPronoun({ formal: true })} is a ${person.parent.getAge()}`;
// Input
let person = {
  firstName: `Ryan`,
  lastName: `Christiani`,
  sayName() {
    return `Hi my name is ${this.firstName} ${this.lastName}`;
  }
};

// Current
let person = {
  firstName: `Ryan`,
  lastName: `Christiani`,
  sayName() {
    return `Hi my name is ${this.firstName} ${this.lastName}`;
  },
};

// Desired - No change
let person = {
  firstName: `Ryan`,
  lastName: `Christiani`,
  sayName() {
    return `Hi my name is ${this.firstName} ${this.lastName}`;
  },
};
// Input
function createMarkup(data) {
  return `
    <article class="pokemon">
      <h3>${data.name}</h3>
      <p>The Pokemon ${data.name} has a base experience of ${data.base_experience}, they also weigh ${data.weight}</p>
    </article>
  `
}

// Current
function createMarkup(data) {
  return `
    <article class="pokemon">
      <h3>${data.name}</h3>
      <p>The Pokemon ${data.name} has a base experience of ${
    data.base_experience
  }, they also weigh ${data.weight}</p>
    </article>
  `;
}

// Desired
function createMarkup(data) {
  return `
    <article class="pokemon">
      <h3>${data.name}</h3>
      <p>The Pokemon ${data.name} has a base experience of ${data.base_experience}, they also weigh ${data.weight}</p>
    </article>
  `;
}
// Input
class element {
  render() {
    return svg`<g>
      ${[0, 10, 20].map((x, i) => svg`<line x1=${x * i} y1="0" x2=${x * i} y2="20"/>`)}
      ${[0, 10, 20].map((y, i) => svg`<line x1="0" y1=${y + i} x2="0" y2=${y + i}/>`)}
    </g>`;
  }
}

// Current
class element {
  render() {
    return svg`<g>
      ${[0, 10, 20].map(
        (x, i) => svg`<line x1=${x * i} y1="0" x2=${x * i} y2="20"/>`
      )}
      ${[0, 10, 20].map(
        (y, i) => svg`<line x1="0" y1=${y + i} x2="0" y2=${y + i}/>`
      )}
    </g>`;
  }
}

// Desired - no change
class element {
  render() {
    return svg`<g>
      ${[0, 10, 20].map(
        (x, i) => svg`<line x1=${x * i} y1="0" x2=${x * i} y2="20"/>`
      )}
      ${[0, 10, 20].map(
        (y, i) => svg`<line x1="0" y1=${y + i} x2="0" y2=${y + i}/>`
      )}
    </g>`;
  }
}
// Input
const render = () => html`
  <ul>
  ${repeat(items, (i) => i.id, (i, index) => html`
    <li>${index}: ${i.name}</li>`)}
  </ul>
`;

// Current
const render = () => html`
  <ul>
  ${repeat(
    items,
    i => i.id,
    (i, index) => html`
    <li>${index}: ${i.name}</li>`
  )}
  </ul>
`;

// Desired - no change
const render = () => html`
  <ul>
  ${repeat(
    items,
    i => i.id,
    (i, index) => html`
    <li>${index}: ${i.name}</li>`
  )}
  </ul>
`;
// Input
const render = () => html`
  ${when(state === 'loading',
  html`<div>Loading...</div>`,
  html`<p>${message}</p>`)}
`;

// Current
const render = () => html`
  ${when(
    state === 'loading',
    html`<div>Loading...</div>`,
    html`<p>${message}</p>`
  )}
`;

// Desired - No change
const render = () => html`
  ${when(
    state === 'loading',
    html`<div>Loading...</div>`,
    html`<p>${message}</p>`
  )}
`;
// Input
const render = () => html`
  <div>Current User: ${guard(user, () => user.getProfile())}</div>
`;

// Current
const render = () => html`
  <div>Current User: ${guard(user, () => user.getProfile())}</div>
`;

// Desired
const render = () => html`
  <div>Current User: ${guard(
    user, 
    () => user.getProfile()
  )}</div>
`;
// Input
const omg = `Covered Lines below threshold: ${
   coverageSettings
.lines

}%. Actual: ${  coverageSummary

.total.

lines
.
pct
  }%`

// Current
const omg = `Covered Lines below threshold: ${
  coverageSettings.lines
}%. Actual: ${coverageSummary.total.lines.pct}%`;

// Desired
const omg = `Covered Lines below threshold: ${coverageSettings.lines}%. Actual: ${coverageSummary.total.lines.pct}%`;
// Input
log(
  chalk.bgRed(
    chalk.white(
      `Covered Lines below threshold: ${coverageSettings.lines}%. Actual: ${coverageSummary.total.lines.pct}%`
    )
  )
);

// Current
log(
  chalk.bgRed(
    chalk.white(
      `Covered Lines below threshold: ${
        coverageSettings.lines
      }%. Actual: ${coverageSummary.total.lines.pct}%`
    )
  )
);

// Desired
log(
  chalk.bgRed(
    chalk.white(
      `Covered Lines below threshold: ${coverageSettings.lines}%. Actual: ${coverageSummary.total.lines.pct}%`
    )
  )
);
// Input
render(html`
  Count: <span>${asyncReplace(countUp())}</span>.
`, document.body);

// Current
render(
  html`
  Count: <span>${asyncReplace(countUp())}</span>.
`,
  document.body
);

// Desired - No change
render(
  html`
  Count: <span>${asyncReplace(countUp())}</span>.
`,
  document.body
);

Playground Link with examples and current output

@lachieh
Copy link

lachieh commented Feb 14, 2018

It's been a month since @JonathanWolfe's suggestion and a few thumbsups on the post. How do we keep it moving? Someone want to do a PR?

@framerate
Copy link

I just started using prettier and to immediately turn it off because of this! Definitely willing to help out if necessary!

@lachieh
Copy link

lachieh commented Mar 20, 2018

*tumbleweeds and crickets*

Would a PR for this be welcomed?

@lydell
Copy link
Member

lydell commented Mar 20, 2018

A PR exploring ways to improve the oh-so-tricky-to-format template literals would certainly be up for discussion 👍

@caub
Copy link

caub commented Apr 6, 2018

@tgroutars
Copy link

Templates strings are still string, and it's helpful to display them as is, hence never ever add a new line. If you need to call a function in a template string, then just declare a variable.
At least having the option to make it behave that way would be great

@ghost
Copy link

ghost commented Nov 8, 2020

most of the time I log, I apply format functions to the content:

console.log(`... ${format(x)} ...`)

which ends up as

console.log(`...${
  format(x)
}...`);

undesirable, IMO. I'd rather have template literals left untouched. It should probably be an option:

{
  singleQuotes: true,
  ...
  ignoreTemplateLiterals: true,
}

@nicu-chiciuc
Copy link

@doplumi If you have a line length of 35 there's definitely no problem. The problem we're facing and I think this issue is about, is when you have a line of 200+ characters which could've been easily split.

@tgroutars
Copy link

@nicu-chiciuc you see what they meant, I don't think they were saying it's a problem with a line length of 35

@terrisgit
Copy link

terrisgit commented Dec 8, 2021

It's difficult to follow all of these posts. What is the behavior for the current version of prettier and are there options to override it? In my case I have a print width of 120 and would like reformatting only of lines that exceed this length and contain a legit ${...}. Prettier seems to be changing every template literal in my .js files to be on a single line regardless of the length (it does respect the backslashes at the end of lines however).

@j-f1
Copy link
Member

j-f1 commented Dec 8, 2021

@terrisgit can you open a new issue by pasting some sample code into the playground, making sure that it demonstrates the undesired behavior you’re seeing, and clicking the “report issue” button at the bottom right? It’s hard to figure out exactly what’s causing the issue unless we can see in detail what’s happening.

@terrisgit
Copy link

@j-f1 The playground link in the first post is pretty good. It doesn't work though.

Error: Invalid proseWrap value. Expected "always", "never" or "preserve", but received true.
    at Ld._applyNormalization (https://prettier.io/lib/standalone.js:1:150240)
    at r (https://prettier.io/lib/standalone.js:1:149535)
    at Ld.normalize (https://prettier.io/lib/standalone.js:1:149575)
    at Yh (https://prettier.io/lib/standalone.js:1:176180)
    at Object.normalizeApiOptions (https://prettier.io/lib/standalone.js:1:177688)
    at normalize (https://prettier.io/lib/standalone.js:1:203877)
    at JD (https://prettier.io/lib/standalone.js:1:227296)
    at Object.formatWithCursor (https://prettier.io/lib/standalone.js:40:40390)
    at formatCode (https://prettier.io/worker.js:141:21)
    at handleMessage (https://prettier.io/worker.js:78:26)

@terrisgit
Copy link

terrisgit commented Dec 8, 2021

@j-f1 Wow the playground is awesome. I think what @Swizec wanted was implemented (why is this issue still open?) ... and now I don't want it. I can just use // prettier-ignore, that's fine.

Another example

@MurzNN
Copy link

MurzNN commented Jan 28, 2022

With simple template strings seems now works well, but if template contains function - Prettier breaks it to several string, here is modified previous example: String with template, containing function

Is this fixable with current version using some configuration options, or maybe some workaround exists?

@masnormen
Copy link

Sadly, despite the "great demand" from the community, the PR #12505 isn't approved. This is still one thing from Prettier that bugs me. Template literals are just that: literals.

If a person doesn't intend to add a newline in a template literal, then, maybe, it should look like there's no new line, no? Or maybe at least give us a choice. Forcing to use concatenation instead of template literals for really long string also isn't always readable.

Despite what you said in the Option Philosophy, I just can hope you'd consider reopening that PR 🙏

@micahbf
Copy link

micahbf commented Nov 17, 2022

Just wanted to chime in here with more support for adding this option.

I am also generally in the camp of less options are better, just give me a reasonable format and don't make me think about it, which is why I love prettier!

But the line splitting on template literals really drives me nuts. It almost always makes the code harder to read and understand. I would really appreciate an option for this behavior!

@andrew-boyd
Copy link

Found this discussion because I would also like to see this option.

@syntastical
Copy link

Yeah, put my in the camp of adding configuration for this. I really dislike the way this is handled.

@adheeshfordev
Copy link

Just adding my name to the people who expect better handling of template literals.

@Matthew-Hiebing
Copy link

Adding my name here too.

@woosuk288
Copy link

OMG, I think many people including me still feel pain from the backtick problem.

Shouldn't Prettier make code prettier?

@bakkot
Copy link
Collaborator

bakkot commented Aug 3, 2023

Here's a heuristic I haven't seen proposed which I think would make ~everyone happy:

Only add linebreaks to an interpolation if there's already a linebreak within the interpolation.

This is similar to how an object expression will get put on multiple lines only if it exceeds print width or contains linebreaks, just with an "and" instead of an "or".

That way if you have a complex interpolation which you want prettier to break up for you, you can put a linebreak in. And if you don't want it broken up - for example, if you wrote the whole template on one line - then prettier will leave it alone.

Thoughts? I'm happy to try a PR if this sounds good.

@bakkot
Copy link
Collaborator

bakkot commented Jan 17, 2024

In version 3.2.0, Prettier incorporates the heuristic I suggested in my previous comment: it will no longer add linebreaks to interpolations in template literals unless there is already one present. So if you want to keep the whole string on a single line, just write it that way, and Prettier will respect that. (It will still format the interpolations as normal, just effectively with infinite printWidth).

This won't make everyone happy, but I hope it's an improvement for most people in this thread.

@masnormen
Copy link

In version 3.2.0, Prettier incorporates the heuristic I suggested in my previous comment: it will no longer add linebreaks to interpolations in template literals unless there is already one present. So if you want to keep the whole string on a single line, just write it that way, and Prettier will respect that. (It will still format the interpolations as normal, just effectively with infinite printWidth).

This won't make everyone happy, but I hope it's an improvement for most people in this thread.

That's a simple yet amazing improvement. Thank you for your work

@Lehoczky-peakfs
Copy link

I wonder if this could be closed since #15209 has been merged. The template literal formatting is great now.

@kachkaev
Copy link
Member

kachkaev commented Jun 3, 2024

@Lehoczky-peakfs, yep, makes sense, thanks! Here is how Prettier 3.3 formats the initial example:

Prettier 3.3.0
Playground link

--parser babel
--trailing-comma none
--prose-wrap always

Input:

class Quote {
    character = {
        name: 'Dorian'
    }

    text = `Yes, ${this.character.name}, you will always be fond of me. I represent to you all the sins you have never had the courage to commit.`
}

Output:

class Quote {
  character = {
    name: "Dorian"
  };

  text = `Yes, ${this.character.name}, you will always be fond of me. I represent to you all the sins you have never had the courage to commit.`;
}

@kachkaev kachkaev closed this as completed Jun 3, 2024
@github-actions github-actions bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Sep 2, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area:template literals lang:javascript Issues affecting JS locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken
Projects
None yet
Development

No branches or pull requests