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

Handlebars: improve conditional formatting #8691

Closed
kangax opened this issue Jul 1, 2020 · 11 comments
Closed

Handlebars: improve conditional formatting #8691

kangax opened this issue Jul 1, 2020 · 11 comments
Labels
lang:handlebars Issues affecting Handlebars (Glimmer) type:bug Issues identifying ugly output, or a defect in the program

Comments

@kangax
Copy link
Contributor

kangax commented Jul 1, 2020

Prettier 2.0.5
Playground link

Input:

{{if condition "foo" "bar" }}
{{if (helper that has a lot of arguments and will likely overflow to the new line) "foo" "bar" }}
{{if (helper that has a lot of arguments and will definitely overflow to the new line) "foo" "bar" }}
{{if condition "longValue1-longValue1__longValue1--longValue1" "longValue2__longValue2--longValue2" }}

Output:

{{if condition "foo" "bar"}}
{{if
  (helper that has a lot of arguments and will likely overflow to the new line)
  "foo"
  "bar"
}}
{{if
  (helper
    that has a lot of arguments and will definitely overflow to the new line
  )
  "foo"
  "bar"
}}
{{if
  condition
  "longValue1-longValue1__longValue1--longValue1"
  "longValue2__longValue2--longValue2"
}}

I think we can (should?) improve the last case where the condition moves to the new line. This, arguably, looks better:

{{if condition
  "longValue1-longValue1__longValue1--longValue1"
  "longValue2__longValue2--longValue2"
}}

I'm not sure how to go about differentiating between which helpers should keep 1st param on the same line (unless it overflows) and which shouldn't. One option is to make a whitelist of helpers like if but that's brittle. Another option is to always leave 1st param on the first line for helpers with 3+ params. That might not be the best formatting for non-conditional helpers with 3+ params. Thoughts?

Another (related) issue I'm seeing on master is with this chunk from our codebase:

Input:

<li class="
{{if showAsCarousel "career-interests__pill--slide-item" "career-interests__pill"}}
"></li>

Output:

<li
  class="
  {{
    if
    showAsCarousel
    "career-interests__pill--slide-item"
    "career-interests__pill"
  }}
  "
></li>

I would expect if to stay adjacent to {{ (looks like #8593 didn't fix all the cases?) Or rather, we seem to be treating helpers different within attributes since this issues doesn't happen in a regular block-level mustache.

@lifeart
Copy link

lifeart commented Jul 1, 2020

related issue: lifeart/vsc-ember-syntax#2

@alexlafroscia
Copy link

Other weird cases that fall into a similar pattern

  {{
    yield
    (hash
      header=(component "fluid-table/thead")
      body=(component "fluid-table/tbody")
      th=(component "fluid-table/th")
      td=(component "fluid-table/td")
    )
  }}

(yield should be in-line with {{, and match even with (hash)

<LinkTo
  @query={{
    hash
    filter=(compute this.computeFilterWithEmail this.filter campaignPic.userEmail)
  }}
  class="text-xs hover:underline"
  data-test-user-email
>
  {{campaignPic.userEmail}}
</LinkTo>

hash should not be on a line all by itself

@thorn0 thorn0 added lang:handlebars Issues affecting Handlebars (Glimmer) type:bug Issues identifying ugly output, or a defect in the program labels Jul 1, 2020
@neojp
Copy link

neojp commented Jul 5, 2020

@alexlafroscia The main issue I've seen about this is that other syntaxes that use mustaches {{ }} tend to have spaces inside as part of their linting process compared to Ember. eg. {{content}}{{ content }} in Vue.

I think it might be why Prettier is biased to render {{yield (hash)}} as:

{{
  yield
  (hash)
}}

I'm not totally against this add spaces inside the mustaches rule, but IMO at the very least (hash) should be indented like this:

{{
  yield
    (hash)
}}

@alexlafroscia
Copy link

alexlafroscia commented Jul 6, 2020

I agree that if {{ and yield cannot be adjacent, that (hash) indented further as you have it would be an improvement.

It's interesting that the Vue community would do something like {{ content }} instead -- admittedly I don't know very much about that tool (aside from it's popularity)

Does Vue have any kind of precedent for what it does to format "helpers" like Ember has, where it's essentially a function invocation within the template? It seems to my like maybe a filter is the closest analogue, which you pipe after a value and follows the same formatting pattern expressed earlier (with whitespace separating values from the curly braces)

@kangax
Copy link
Contributor Author

kangax commented Jul 8, 2020

@alexlafroscia yep, I can reproduce yield shifting onto the new line only when it's in an attribute, similar to hash, if and any other helper:

<div
  foo={{
    yield
    (hash foo bar="string that's long enough to definitely create a new line")
  }}
></div>

@elwayman02
Copy link

I would argue in favor of the following:

{{if condition
  "long value"
  "long value 2"
}}

as well as

{{yield (hash
  foo='long bar string'
)}}

I think putting yield on a line by itself looks pretty awful and wastes a lot of vertical space. I would liken it to JS, where we do the following:

list.forEach(function () {
  //do stuff
});

rather than

list.forEach(
  function () {
    //do stuff
  }
);

@jelhan
Copy link

jelhan commented Nov 20, 2020

I encountered another edge case if a helper exceeds the print width only a little bit.

Input:

{{if abcdefghijklmnopqrstuvwxyz (t 'abcdefghijklmnopq') (t 'abcdefghijklmnopq')}}

{{#if (eq abcdefghijklmnopqrstuvwxyzabcdefgh abcdefghijklmnopqrstuvwxyzabcdefgh)}}
foo{{/if}}

{{some-helper abcdefghijklmnopqrstuvwxyz abcdefghijklmnopqrstuvwxyz abcdefghijkl}}

Output:

{{if abcdefghijklmnopqrstuvwxyz (t "abcdefghijklmnopq") (t "abcdefghijklmnopq")
}}

{{#if (eq abcdefghijklmnopqrstuvwxyzabcdefgh abcdefghijklmnopqrstuvwxyzabcdefgh)
}}
  foo
{{/if}}

{{some-helper abcdefghijklmnopqrstuvwxyz abcdefghijklmnopqrstuvwxyz abcdefghijkl
}}

Expected:

{{if
  abcdefghijklmnopqrstuvwxyz
  (t "abcdefghijklmnopq")
  (t "abcdefghijklmnopq")
}}

{{#if
  (eq abcdefghijklmnopqrstuvwxyzabcdefgh abcdefghijklmnopqrstuvwxyzabcdefgh)
}}
  foo
{{/if}}

{{some-helper
  abcdefghijklmnopqrstuvwxyz
  abcdefghijklmnopqrstuvwxyz
  abcdefghijkl
}}

Please note that it works for all as expected if adding a few more chars. This issue only occurs if the helper invocation without closing brackets does not exceed print width.

Playground: https://prettier.io/playground/#N4Igxg9gdgLgprEAuExgEsBmACAhgIzABM5MBzAC3QCsBrAGwFsoIAHARwCcBnGAVwBuAdwAeATwBe2ABQxsAcgLFSlGg2Zt28gJQy5iwiXJU6TFhx0BfSwB0odtAGIsMuOzyGVJ9ea69BopJKRpQeysZqZpo8-MLiEsFe2tZ2mBAQaAD0WCn2UGjcEIxwALQUcPSscJxhId5RHDEB8bVekRqN-nGSrRGm1iAANCBsMOjQ3MiguJycEEIACjMIkyi49EK4YpPD+Jy4YLRwMADKrAfoUGTIMJx8cMNwjPhwRCREADK4V3y4ZHAAMQgnEYuBgYyuyBAuD4MAgQxAFBgjHoAHUqPBuOcwHATit0GMBASxFCwNwdiBLtxqjAFvsyKDkJh1tThtRuCIAEL7Q7HE64YofS5wJksh4gdkiE6XMj0OAART4EHgovorJA5x41ShsvQjGKnARrE4lxgqPQRBgFGQAA4AAzDY0Qamo-asKHGuDUzgCEXDdhK+B0tiraHcEpQOCvV4IzhuPjoON0v6MpDMtXi6mMdA3O6ZmVyxXKkVpsXDGAEc2W61IABM5f26HoMoAwkVUyAvQBWBF8akAFQIq3T6oE9wAklASLATmATawYABBKcnGBiOWq6nWIA

@kangax
Copy link
Contributor Author

kangax commented Dec 1, 2020

@jelhan yep, I came across that one before; see #6 in jgwhite#1 (comment)

@Alonski
Copy link
Contributor

Alonski commented Dec 29, 2020

Also running into this :)
Long if statements on new lines

@chriskrycho
Copy link

Should this be closed? Looks like all the cases described here are fixed via #10586!

@alexlafroscia
Copy link

alexlafroscia commented May 12, 2021

Running prettier@2.3 on our app this morning resulted in much better conditional formatting 🎉 I'm satisfied with the result!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang:handlebars Issues affecting Handlebars (Glimmer) type:bug Issues identifying ugly output, or a defect in the program
Projects
None yet
Development

No branches or pull requests

10 participants