Glimmer: fix newlines rendering in non-mustache attributes #8667

kangax · 2020-06-29T17:41:34Z

It appears that hardline "primitive" can't be rendered within an attribute in Prettier and results in \n output as [object Object].

When working on a fix for this, I found out that Glimmer parser treats attribute values with mustaches differently: they're a ConcatStatement rather than a single TextNode if the attribute value is a plain string.

This PR changes TextNode output behavior to only trim it if it's the only node within an attribute (as opposed to ConcatStatement).

I’ve added tests to confirm my change works.
(If the change is user-facing) I’ve added my changes to changelog_unreleased/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
I’ve read the contributing guidelines.

✨Try the playground for this PR✨

kangax · 2020-06-29T17:44:55Z

@dcyriller I saw you were an author of this part of code. Let me know what you think! My first PR to prettier so might be rough around the edges :)

fisker · 2020-06-30T11:39:56Z

We don't trim attribute in html

Prettier pr-8667
Playground link

--parser html

Input:

<a title="
attr
"></a>

Output:

<a
  title="
attr
"
></a>

dcyriller · 2020-06-30T11:40:02Z

src/language-handlebars/printer-glimmer.js

+      // they're a ConcatStatement rather than a single TextNode
+      // if the attribute value is a plain string, we only trim it and avoid inserting hardlines
+      // since they're not rendered correctly within attributes
+      if (isParentOfSomeType(path, ["AttrNode"])) {


This conditionnal will match <htmlElement attribute="textNode">'s TextNode but not <htmlElement attribute="textNodeInConcatStatement {{mustache}}">'s.

I would suggest to use instead:

Suggested change

if (isParentOfSomeType(path, ["AttrNode"])) {

if (path.stack.includes("attributes")) {

to match both.

If you apply this suggestion, you'll notice that you can return return concat([trimmedValue]); from line 234 and get rid of the big else statement.

Doing that, we'd split the TextNode printer in two: one for TextNodes nested in attributes, one for other TextNodes. We'd be able later to print differently attributes whether they be class / style / srcset / other.

This conditionnal will match <htmlElement attribute="textNode">'s TextNode but not <htmlElement attribute="textNodeInConcatStatement {{mustache}}">'s.

Correct and that's the whole point — presence of MustacheStatement makes it ConcatStatement which is handled properly.

It is handled correctly in the sense it doesn’t have the bug. But a TextNode in a ConcatStatement should probably be formatted no differently than a simple TextNode (ie. trimmed / not trimmed)?

@dcyriller just tried your suggestion and remembered why I didn't do that. It breaks mustache handling in attributes:

<div - class='a-first-class - {{if this.optionOne 'optionOne'}} - - {{if this.optionTwo 'optionTwo'}} - - {{if this.optionThree 'optionThree'}} - - {{if this.optionFour 'optionFour'}} - - {{if this.optionFive 'optionFive'}} - - {{this.class}}' + class='a-first-class {{if this.optionOne 'optionOne'}} {{ + if this.optionTwo 'optionTwo' + }} {{if this.optionThree 'optionThree'}} {{ + if this.optionFour 'optionFour' + }} {{if this.optionFive 'optionFive'}} {{this.class}}' ...attributes > </div>

This ☝️ would actually be an improvement - if applied to only class names.

Yeah, this is something I wanted to discuss with you too. In #6207 they ask to format classes with newlines (but not extra newlines) whereas I feel like ☝️ option is not bad. We could consider that ticket an improvement or bring it up for more discussion.

tests/handlebars/whitespace/__snapshots__/jsfmt.spec.js.snap

dcyriller · 2020-06-30T11:48:00Z

@fisker handlebars printer for attributes is not yet splitted: we could rewrite whitespaces in class and style attributes, print scrset in a custom way and do not process other attributes. This is what html printer does? What do you think?

fisker · 2020-06-30T11:55:34Z

@dcyriller I'm not familiar with handlebars, html didn't trim attribute, I think we should not do trim in handlebars either? And yes, html format style class and srcset.

kangax · 2020-06-30T16:19:41Z

@fisker @dcyriller Ah, I missed the fact that Prettier treats attribute values differently depending on an attribute. I initially skipped trimming entirely but then noticed a failing glimmer test that checked for class=" smth " being trimmed to class="smth". I then saw in the playground that class attribute does indeed get trimmed in, at least, html printer.

Looking at something like this:

<input
class="
class"
style="
foo:bar"
srcset="
srcset"
title="
title"
alt="
alt"
unspecified="
unspecified"
/>

I see that "html" printer only trims class and style?

<input
  class="class"
  style="foo: bar;"
  srcset="
srcset"
  title="
title"
  alt="
alt"
  unspecified="
unspecified"
/>

JSX printer seems to leave everything untouched:

<input
  class="
class"
  style="
foo:bar"
  srcset="
srcset"
  title="
title"
  alt="
alt"
  unspecified="
unspecified"
/>;

I can skip trimming for now entirely, so that we get consistent behavior and fix this bug, then work on splitting printer behavior depending on an attribute?

fisker · 2020-06-30T16:32:47Z

srcset has own parser

Prettier 2.0.5
Playground link

--parser html

Input:

<img
class="
class"
style="
foo:bar"
srcset="
srcset------------------------------------------------------------ 1x,
        srcset_2  2x"
title="
title"
alt="
alt"
unspecified="
unspecified"
/>

Output:

<img
  class="class"
  style="foo: bar;"
  srcset="
    srcset------------------------------------------------------------ 1x,
    srcset_2                                                           2x
  "
  title="
title"
  alt="
alt"
  unspecified="
unspecified"
/>

kangax · 2020-06-30T16:35:18Z

We can look at cleanAttributeValue from my html-minifier (haven't worked on it in a while, forgot the rules) for some inspiration :) https://github.com/kangax/html-minifier/blob/gh-pages/src/htmlminifier.js#L257-L328

kangax · 2020-07-13T15:34:29Z

@dcyriller would you like me to wait until you merge your PR and then we rebase this one on top? or should we merge this as is and then follow up with yours?

dcyriller · 2020-07-16T16:40:35Z

src/language-handlebars/printer-glimmer.js

+        .replace(/^\s+/g, leadingSpace)
+        .replace(/\s+$/, trailingSpace);
+
+      // Glimmer parser treats attribute values with mustaches differently


This comment might removed - it doesn't explain how prettier printer works (but the undelying ast).

dcyriller · 2020-07-16T16:40:59Z

tests/handlebars/whitespace/__snapshots__/jsfmt.spec.js.snap

@@ -406,7 +406,6 @@ sometimes{{nogaps}}areimportant<Hello></Hello>
 <div class="a list of classes">

 </div>


This change might be reverted

dcyriller · 2020-07-16T16:45:21Z

@dcyriller would you like me to wait until you merge your PR and then we rebase this one on top? or should we merge this as is and then follow up with yours?

@fisker what do you think?

IMHO if this is merged, I can rebase and add more tests on #8677 (as the bug would be fixed)

fisker · 2020-07-16T16:56:32Z

@dcyriller This PR prints trimmed attribute values, it's not okay right? So we can't merge this one.

dcyriller · 2020-07-16T19:08:16Z

The other one - to format only class names - is ready 👍.

kangax · 2020-07-17T14:57:11Z

@dcyriller ok, waiting for yours to merge before we go ahead with this one

Alonski · 2020-07-29T06:27:36Z

@kangax This was merged :)
#8677

kangax · 2020-08-03T18:27:00Z

@Alonski @dcyriller any idea why the tests are failing on CI? everything passes locally, ran yarn test -u and so on

fisker · 2020-08-03T22:42:43Z

@kangax the test fails because you changed the AST, need fix it in clean, to test locally, AST_COMPARE=1 yarn test

fisker · 2020-08-04T12:55:28Z

src/language-handlebars/clean.js

@@ -6,7 +6,7 @@ module.exports = function (ast, newNode) {

  // (Glimmer/HTML) ignore TextNode whitespace
  if (ast.type === "TextNode") {
-    const trimmed = ast.chars.trim();
+    const trimmed = ast.chars.trim().replace(/\s+/g, " ");


Please, only do nessary change, should check attribute name.

@fisker actually I don't quite understand why we need to do this. Could you explain? Why are we "cleaning" AST and when does this happen? Also, why does the test fail now if we only changed Prettier output and not the underlying AST? Checking for attribute==class will involve some duplication from Glimmer's printer which I'd rather avoid. I'm also not sure if this place is performance critical?

This only runs when comparing the AST during test, so performance is not very important, but make sure the formated / original code have the same AST is very important.

Also, why does the test fail now if we only changed Prettier output and not the underlying AST?

You changed the output, it parse to different ASTs.

I'm not familiar with glimmer AST , but I checked failed tests on CI , the attribute with TextNode have "name": "class", can we use it ?.

…ettier into glimmer_newlines_in_attributes

kangax added 4 commits June 29, 2020 12:49

Glimmer: do not insert hardlines in attributes

4085582

Add a changelog file

bb6da7d

Update PR name in changelog

d751a75

Update PR number

6e78353

Rename PR file

6d90950

alexander-akait approved these changes Jun 30, 2020

View reviewed changes

dcyriller reviewed Jun 30, 2020

View reviewed changes

tests/handlebars/whitespace/__snapshots__/jsfmt.spec.js.snap Outdated Show resolved Hide resolved

dcyriller mentioned this pull request Jun 30, 2020

handlebars: Do not format html element attributes but class #8677

Merged

4 tasks

kangax added 2 commits July 13, 2020 11:28

Move tests to a separate file

bcebaa5

Add a test file

0151865

dcyriller reviewed Jul 16, 2020

View reviewed changes

kangax added 4 commits August 3, 2020 07:24

Merge master

0bc5bef

Remove unrelated change

ecb587c

Remove unrelated change

9efaf73

Fix behavior after rebase

285ca50

Collapse class whitespace in AST

87c3de4

fisker reviewed Aug 4, 2020

View reviewed changes

kangax added 15 commits August 28, 2020 11:29

Move tests to a separate file

16a8e94

Glimmer: do not insert hardlines in attributes

bfb4d8b

Add a changelog file

2e11a38

Update PR name in changelog

9e41cb5

Update PR number

e9df64c

Rename PR file

8ee634b

Move tests to a separate file

8f0f9b0

Add a test file

1765fd3

Remove unrelated change

56a50d5

Remove unrelated change

def79b6

Fix behavior after rebase

ababa5d

Collapse class whitespace in AST

d3a6c0a

Move tests to a separate file

b83a238

Merge branch 'glimmer_newlines_in_attributes' of github.com:kangax/pr…

c4dcf0a

…ettier into glimmer_newlines_in_attributes

Revert whitespace

6f83fd0

Base automatically changed from master to main January 23, 2021 17:13

thorn0 mentioned this pull request Jan 26, 2021

Handlebars: fix formatting of attributes #10145

Merged

3 tasks

thorn0 closed this in #10145 Jan 28, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jan 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Glimmer: fix newlines rendering in non-mustache attributes #8667

Glimmer: fix newlines rendering in non-mustache attributes #8667

kangax commented Jun 29, 2020 •

edited by fisker

kangax commented Jun 29, 2020

fisker commented Jun 30, 2020

dcyriller Jun 30, 2020

dcyriller Jun 30, 2020

kangax Jun 30, 2020 •

edited

dcyriller Jun 30, 2020

kangax Jul 13, 2020

dcyriller Jul 16, 2020

kangax Jul 17, 2020

dcyriller commented Jun 30, 2020

fisker commented Jun 30, 2020

kangax commented Jun 30, 2020

fisker commented Jun 30, 2020 •

edited

kangax commented Jun 30, 2020

kangax commented Jul 13, 2020

dcyriller Jul 16, 2020

dcyriller Jul 16, 2020

dcyriller commented Jul 16, 2020 •

edited

fisker commented Jul 16, 2020

dcyriller commented Jul 16, 2020

kangax commented Jul 17, 2020

Alonski commented Jul 29, 2020

kangax commented Aug 3, 2020

fisker commented Aug 3, 2020 •

edited

fisker Aug 4, 2020

kangax Aug 4, 2020

fisker Aug 4, 2020

fisker Aug 4, 2020

fisker Aug 4, 2020

	if (isParentOfSomeType(path, ["AttrNode"])) {
	if (path.stack.includes("attributes")) {

		@@ -406,7 +406,6 @@ sometimes{{nogaps}}areimportant<Hello></Hello>
		<div class="a list of classes">

		</div>

Glimmer: fix newlines rendering in non-mustache attributes #8667

Glimmer: fix newlines rendering in non-mustache attributes #8667

Conversation

kangax commented Jun 29, 2020 • edited by fisker

kangax commented Jun 29, 2020

fisker commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kangax Jun 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcyriller commented Jun 30, 2020

fisker commented Jun 30, 2020

kangax commented Jun 30, 2020

fisker commented Jun 30, 2020 • edited

kangax commented Jun 30, 2020

kangax commented Jul 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcyriller commented Jul 16, 2020 • edited

fisker commented Jul 16, 2020

dcyriller commented Jul 16, 2020

kangax commented Jul 17, 2020

Alonski commented Jul 29, 2020

kangax commented Aug 3, 2020

fisker commented Aug 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kangax commented Jun 29, 2020 •

edited by fisker

kangax Jun 30, 2020 •

edited

fisker commented Jun 30, 2020 •

edited

dcyriller commented Jul 16, 2020 •

edited

fisker commented Aug 3, 2020 •

edited