[Glimmer] 🐛 Linebreak added around closing tag and text comming just after

[bartocc]

Prettier 2.0.5
Playground link

--parser glimmer

Input:

<span>foo</span>bar

Output:

<span>
  foo
</span>
bar

Expected behavior:

<span>foo</span>bar

Formatting with --parser html produces the expected output. See this playground link

When rendered, the browser displays foo bar while it should display foobar

[kangax]

@dcyriller do you know if we're able to reuse some of html printer behavior in glimmer? is this a common thing? I think in this case we could mostly just use HTML semantics and take special care of things like adjacent mustache statements.

[dcyriller]

This was my first idea and it would be awesome because most of the work to solve this issue would be done. But unfortunately html parser doesn’t support handlebars syntax (especially modifiers). So we can’t embed html printer in handlebars. Or am I missing something?

[kangax]

I went down the rabbit hole of figuring out how this should work 😁 Found few things. HTML parser is quite different from the JSX one. HTML parser also has 3 whitespace modes (css, strict, ignore) which gives us 4 different outputs to compare to, yay.

Here's some sample input of various mixed content (first 4 in 1-line form, last 4 in indented form):

<div>
<div>hello<span>world</span></div>
<div><span>hello</span>world</div>
<div><span><input type="text" /></span></div>
<div>just text</div>
<div>
  hello<span>world</span>
</div>
<div>
  <span>hello</span>world
</div>
<div>
  <span>
    <input type="text" />
  </span>
</div>
<div>
  just text
</div>
</div>

We get this output:

JSX

<div>
  <div>
    hello<span>world</span>
  </div>
  <div>
    <span>hello</span>world
  </div>
  <div>
    <span>
      <input type="text" />
    </span>
  </div>
  <div>just text</div>
  <div>
    hello<span>world</span>
  </div>
  <div>
    <span>hello</span>world
  </div>
  <div>
    <span>
      <input type="text" />
    </span>
  </div>
  <div>just text</div>
</div>;

HTML (css)

Notable difference is that it doesn't try to format child tags unless they're already formatted.

<div>
  <div>hello<span>world</span></div>
  <div><span>hello</span>world</div>
  <div>
    <span><input type="text" /></span>
  </div>
  <div>just text</div>
  <div>hello<span>world</span></div>
  <div><span>hello</span>world</div>
  <div>
    <span>
      <input type="text" />
    </span>
  </div>
  <div>
    just text
  </div>
</div>

HTML (strict)

Mostly the same as "css" except all whitespace is significant.

<div>
  <div>hello<span>world</span></div>
  <div><span>hello</span>world</div>
  <div
    ><span><input type="text" /></span
  ></div>
  <div>just text</div>
  <div> hello<span>world</span> </div>
  <div> <span>hello</span>world </div>
  <div>
    <span>
      <input type="text" />
    </span>
  </div>
  <div>
    just text
  </div>
</div>

HTML (ignore)

This looks remarkably similar to JSX in that it breaks all nested tags except JSX does it in a safer way. Notice, e.g. mixed content (hello<span>world</span>) being preserved in JSX but not here.

<div>
  <div>
    hello
    <span>world</span>
  </div>
  <div>
    <span>hello</span>
    world
  </div>
  <div>
    <span><input type="text" /></span>
  </div>
  <div>just text</div>
  <div>
    hello
    <span>world</span>
  </div>
  <div>
    <span>hello</span>
    world
  </div>
  <div>
    <span>
      <input type="text" />
    </span>
  </div>
  <div>
    just text
  </div>
</div>

Now the question is — what should glimmer do?

There are few ways to go about it. We could just follow what JSX does. JSX printer looks like a good compromise between html-ignore-whitespace (that looks too lax / broken up) and html-default-css (that looks too squished in cases when lines are not already formatted).

We could follow (default, css) HTML implementation. I worry about cases where tags' default behavior is overwritten and it'd break formatting, the whole reason why Prettier has "strict" mode.

Another option is to try to completely follow HTML parser behavior, including support for css/strict/ignore flags. This would also be the hardest and longest way.

I quite like going the JSX route, at least for the MVP version — it'd provide consistent breaks while being conservative with the whitespace.

Thoughts?

[kangax]

Here's a draft PR that fixes most (all?) of whitespace issues but collapses all tags into 1 line as long as they fit. Probably not the formatting behavior we're looking for but it does result in a significantly more correct rendering.

<div>
  <div>
    hello
    <span>world</span>
  </div>
  <div>
    <span>hello</span>
    world
  </div>
  <div><span><input type="text" /></span></div>
  <div>just text</div>
  <div>
    hello
    <span>world</span>
  </div>
  <div>
    <span>hello</span>
    world
  </div>
  <div><span><input type="text" /></span></div>
  <div>just text</div>
</div>

[thorn0]

@kangax Formatting hello<span>world</span> to

    hello
    <span>world</span>

doesn't look like being conservative with the whitespace.

Did you read the explanation of the --html-whitespace-sensitivity option? If in Glimmer, whitespace works the same way it does in "normal" HTML, then this option with its 3 values needs to be implemented for Glimmer too.

[kangax]

@thorn0 JSX seems to strike a good balance between formatting and correctness; it breaks tags stuffed on one line, for example, without breaking whitespace (<span>hello</span>world). AFAIU, if we follow JSX behavior, we can avoid the complexity of having 3 flags.

I just noticed that my draft PR breaks <span>hello</span>world — is this what you meant? Yeah, that's no good, I'll look into it.

[thorn0]

@kangax JSX has completely different rules for whitespace handling. They can't be used for HTML.

[boris-petrov]

@kangax - my personal view is that Prettier is a code formatter and nothing else. In that sense it should not break code. That means when formatting JavaScript, to not output code that has a different meaning. Same for HTML - it shouldn't generate a result that has different semantics from the original. So the output I've given in the other issue is, for me, absolutely unacceptable.

I think the best way, obviously, is to support the same options as the html parser - namely htmlWhitespaceSensitivity. Of course, that's most likely a ton of work which none of you/us can invest, so for me the right way is to just choose one of the variants and hardcode it - namely strict. That preserves the semantics as best as possible and will not break people's code (which is a good thing if you format 1000 files and you don't have 100% test coverage with screenshots for everything to check for regressions). That way people can write whitespace-sensitive HTML and not worry that the formatter will break it by rearranging stuff.

[dcyriller]

I like the idea of changing the mode of handlebars printer from htmlWhitespaceSensitivity: ignore to strict. It should be a reasonable amount of work (compared to developing the css mode). And it would make Prettier for handlebars much more usable. Also it shouldn’t make the development of a css mode harder.

[jede]

I think one approach could be to respect <!-- display: inline --> until css mode is done? Right now it seems to be ignored. Playground

[mamiller93]

Any updates to this? Or any workarounds other than not using the parser?

[kangax]

@mamiller93 No workarounds that i'm aware of. This is still on my radar as that's one of the blockers for us to start using this internally at LinkedIn. I've been caught up with other work stuff and haven't had time to work on it.

[dcyriller]

@kangax FYI I implemented the --htmlWhitespaceSensitivity=strict option for Handlebars in #9885. The formatting should be correct. But it can be improved.

[dcyriller]

#9885 might close this issue? 🤔

Playground link for the PR

[thorn0]

Okay, let's consider this fixed.

[bartocc]

thx for the work on this @dcyriller 👍

[kangax]

@dcyriller amazing work, thank you! once this makes it to the latest prettier, i'll give it a try over our codebase at LinkedIn