[QUEST] Glimmer

[jgwhite]

Objective

We want to get Prettier’s support for Ember templates up to the point where it is complete and maintainable enough to be officially released.

This quest issue was created to track the outstanding work required to meet this objective and to help volunteers coordinate to get the work done.

Contributing

The easiest way to contribute is to try Prettier’s Ember template support out in your apps and report any unwanted, incomplete, or otherwise wonky behaviour as a comment on this issue so it can be added to the quest. To do this:

$ yarn add -D prettier
$ yarn prettier '**/*.hbs' --parser glimmer

Or

$ npm i --save-dev prettier
$ npm run prettier '**/*.hbs' --parser glimmer

Then check the changes and look for stuff that makes you frown!

When you’re ready to contribute some code, identify an unowned item on the list below and leave a comment stating that you intend to work on it. We’ll tag the item with your handle. Once you have a PR that we can reference, leave another comment so we can link the item to the PR.

If you decide for whatever reason that you no longer wish to pursue the work, leave another comment and we’ll remove your handle from it. This is absolutely fine and you shouldn’t feel any regret if things don’t pan out.

Tasks

Check bugs on Prettier labeled with "lang:handlebars".

Why does this quest issue live on a fork of prettier/prettier?

We’re trying it out as an experiment so we can easily add administrators on this issue.

[rwjblue]

RE: the entity escaping, we really need to avoid the escaping all together (see https://github.com/prettier/prettier/pull/4533#issuecomment-495666923 for more details)

[GavinJoyce]

I'm working on fixing the HTML entities here: https://github.com/prettier/prettier/pull/6234

[tchak]

@jgwhite if you add me to this fork it will be easier for me to copy the source markdown to the upstream issue

[GavinJoyce]

Perhaps it would be better to just have a single quest issue? Perhaps we should just use this one as we can have multiple editors? We could link to this issue from https://github.com/prettier/prettier/issues/4908

@jgwhite perhaps you could give me edit access to this issue too?

[tchak]

I just linked the main issue to this one

[dcyriller]

I've opened a PR to fix a bug: prettier is a bit eager regarding whitespaces between text and helpers https://github.com/prettier/prettier/pull/6239

// Input
{{#component propA}}
    for {{propB}}  do {{propC}} f
{{/component}}

// Output (Prettier stable)
{{#component propA}}
  for{{propB}}do{{propC}}f
{{/component}}

// Output (Prettier the PR)
{{#component propA}}
  for {{propB}}  do {{propC}} f
{{/component}}

@GavinJoyce I hope it won't conflict with what your current work 😅

[Alonski]

@dcyriller Do you rely on this whitespace? I personally would like a more verbose way to specify significant whitespace. Maybe I'm wrong 😅

[jgwhite]

@dcyriller added to the tracking info description. Nice one!

Personally I’d like Prettier to behave like this:

Before:

<p>Hello   {{name}}   happy   {{age}}    birthday!</p>

<pre>
  Name: {{name}}
  Age:  {{age}}
</pre>

After:

<p>Hello {{name}} happy {{age}} birthday!</p>

<pre>
  Name: {{name}}
  Age:  {{age}}
</pre>

i.e. don’t mess with whitespace instead <pre> tags.

Edit:

Though it now occurs to me that there’d be no way for Prettier to know about components like:

<CodeBlock @format="plain">
  Name: {{name}}
  Age:  {{age}}
</CodeBlock>

[GavinJoyce]

Though it now occurs to me that there’d be no way for Prettier to know about components like:

I think we'll need to add support for {{!-- prettier-ignore --}}:

{{!-- prettier-ignore --}}
<CodeBlock @format="plain">
  Name: {{name}}
  Age:  {{age}}
</CodeBlock>

[dcyriller]

Reading your comments I understand we should have:

// Input
{{#component propA}}
    for {{propB}}  do {{propC}} f
{{/component}}

// Output (Prettier stable)
{{#component propA}}
  for{{propB}}do{{propC}}f
{{/component}}

// Output (Prettier the PR)
{{#component propA}}
  for {{propB}} do {{propC}} f
{{/component}}

I'll update my PR tomorrow to reflect that!

Also, we should implement {{!-- prettier-ignore --}} for handlebars (most probably in an other PR).

Thank you for your feedbacks!

EDIT: just saw that you opened a new PR two hours before mine @GavinJoyce 😬. So, I'll close it for now. Once yours is merged, I'll be able to rebase and open mine (if still necessary).

[GavinJoyce]

@dcyriller please feel free to land anything you like, I'm going to switch focus this weekend and try an experiment to rebuild prettier's glimmer printer from first principles. I doubt that it will come to anything, but I think it will be useful in learning prettier's internals.

[jgwhite]

@GavinJoyce I sense a future emberconf talk in the making…

[ghost]

@GavinJoyce I'm in the peanut gallery here, but I'm curious if there is any overlap with @rwjblue 's ember-template-recast. If anything another implementation to compare.

[jgwhite]

And also @code0100fun’s ember-template-rewrite.

[dcyriller]

I opened a PR to add trailing newline to hbs files: https://github.com/prettier/prettier/pull/6243 (my IDE adds a final newline on every opened files so I had git changes every time I opened a file 😛. Also, Prettier adds final newline for every other language.)

[jgwhite]

@dcyriller fantastic. This is one of the major roadblocks we ran into as well.

[jgwhite]

I wonder if it’d be worth announcing this quest in the Ember Times? It’d be awesome to get the community trying out the current glimmer support and shaking loose more todos for the quest.

[Alonski]

@jgwhite Good idea :) I will see if anyone can write something up for this. Would you be willing to write a short section?

[laurmurclar]

Is there an expected timeline for this work? I'd love to work on "Improve curly/text spacing" but it'll probably be the weekend before I could make significant progress on it

[jgwhite]

Is there an expected timeline for this work?

@laurmurclar no timeline, please jump in! 👍

I'd love to work on "Improve curly/text spacing" but it'll probably be the weekend before I could make significant progress on it

I’ll tag that item with your name. @dcyriller are you happy with that?

[dcyriller]

I’ll tag that item with your name. @dcyriller are you happy with that?

Sure thing. Also, I think there are a few things to fix around curly/text spacing.

[jgwhite]

Now this has appeared in the Ember Times I’ve added some more guidance to the readme. @GavinJoyce would you mind adding a line or two on how to use the Prettier playground for Glimmer?

[dcyriller]

FWIW I’ve run Prettier against two codebases. You can find these examples here: https://github.com/dcyriller/ember-template-lint-plugin-prettier#examples

[samselikoff]

Trying this out on some templates in EmberMap's codebase, very nice work y'all!

Biggest unwanted behavior is definitely (1) single blank newlines used for visual clarity and (2) single blank spaces between words and expressions.

Before:

{{ui-video-hero
  video=model
  on-end=(action 'playNextVideo')
  on-play=(action 'turnOnAutoplay')
  on-pause=(action 'turnOffAutoplay')}}

{{#ui-container}}
  <main class="my-4 pb-2 lg:flex justify-between">
    <article class="w-full lg:w-3/5 flex-no-shrink">
      {{#ui-title style='headline'}}
        {{model.title}}
      {{/ui-title}}

      {{#ui-p data-test-id="series-description"}}
        {{model.description}}
      {{/ui-p}}

      {{video-extras clip=model belongsToSeries=true}}
    </article>

    {{#if screen.isLargeAndUp}}
      <div class="w-2/5 max-w-390">
        <div class="mb-4 pl-3">
          {{topics/topic/video/components/series-playlist
            series=model.series
            activeClip=model}}
        </div>
      </div>
    {{/if}}

  </main>
{{/ui-container}}

{{questions-pancake}}

After:

{{ui-video-hero
  video=model
  on-end=(action "playNextVideo")
  on-play=(action "turnOnAutoplay")
  on-pause=(action "turnOffAutoplay")
}}
{{#ui-container}}
  <main class="my-4 pb-2 lg:flex justify-between">
    <article class="w-full lg:w-3/5 flex-no-shrink">
      {{#ui-title style="headline"}}
        {{model.title}}
      {{/ui-title}}
      {{#ui-p data-test-id="series-description"}}
        {{model.description}}
      {{/ui-p}}
      {{video-extras clip=model belongsToSeries=true}}
    </article>
    {{#if screen.isLargeAndUp}}
      <div class="w-2/5 max-w-390">
        <div class="mb-4 pl-3">
          {{topics/topic/video/components/series-playlist
            series=model.series
            activeClip=model
          }}
        </div>
      </div>
    {{/if}}
  </main>
{{/ui-container}}
{{questions-pancake}}

Everything else seems to be pretty spot on!

[laurmurclar]

Very sorry folks, haven't had much of a chance to work on this! I probably won't for a while either. Mind removing my name from that task? 😞

[jgwhite]

@laurmurclar no problem at all, and no need to apologize.

[chadian]

https://github.com/prettier/prettier/pull/6354

Addresses a bunch of the whitespace and line break issues. The changes are probably best reflected in the jest snapshots. I also added a test case preserved-spaces-and-breaks.hbs to try and capture a few different combinations and edge cases. Existing tests were changed to preserve intentional line breaks similar to what @samselikoff mentions:

(1) single blank newlines used for visual clarity

Give it a try. I would love any feedback!

[samselikoff]

@chadian Very nice man! This takes care of all the issues I had last time I tried it 👏

Bug in my Atom editor integration that won't let me format-on-save, but once I do I think I'll make a video on this. Super exciting everyone, great stuff.

[FabHof]

I can have a look at the line breaks of dynamic element class names.

[dcyriller]

@jgwhite here are a few recent developments that you may want to reference in the description:

a small bug fix: https://github.com/prettier/prettier/pull/6377 (it applies --single-quote option in AttrNode)

a test only PR: https://github.com/prettier/prettier/pull/6378

a small issue to tackle (probably related to the unwanted line breaks within dynamic element class name): https://github.com/prettier/prettier/issues/6252

[jgwhite]

@dcyriller awesome. I’ve given you admin right on this fork — would you mind updating the issue description?

[samselikoff]

Quick question, has anybody been able to use this with format-on-save in Prettier?

[Alonski]

@samselikoff You use Atom right? There is a VSCode extension: https://marketplace.visualstudio.com/items?itemName=mfeckies.handlebars-formatter

[samselikoff]

Ya Atom

[FabHof]

I did some work on the spaces in dynamic element class name: prettier/prettier#6464

[chadian]

Improve whitespace handling (@chadian) has been merged. @jgwhite Can you check it off on the list for me? Thanks!

[xomaczar]

@jgwhite I think @chadian also fixed Improve curly/text spacing with his latest PR.

[samselikoff]

I'd like to try out the latest round of improvements! I'm using VSCode now but the two marketplace extensions have prettier pinned I believe, so I'm trying to get it setup on my own to no avail.

Basically tried installing master of Prettier and adding this to my project

module.exports = {
  overrides: [
    {
      files: "*.hbs",
      options: { parser: "glimmer" }
    }
  ]
};

but code is telling me there's no formatter installed for 'handlebars' files. Anyone have any tips?

[jgwhite]

@samselikoff until Glimmer is officially supported you’ll need a prettier handlebars VSCode extension like this one.

[samselikoff]

@jgwhite I see, thanks. Do you know if it's possible to use an extension like that but also use prettier#master so I can try out these updates?

I did install master into my project and with the override above, yarn prettier welcome.hbs worked (it used the glimmer parser for my hbs file).

[jgwhite]

@samselikoff 🤔 I’m not sure. It’s a shame the extensions are architected that way.

[samselikoff]

Not exactly sure what I changed but https://github.com/mfeckie/handlebars-formatter seems to be using some of the new whitespace improvements.

Before:

After:

Looks amazing, nice work everyone!

I'm also using the "Prettier - Code formatter" extension which was giving me trouble, so I had to add this to my settings.json to tell Code to use this extension for handlebars files instead:

"[handlebars]": {
  "editor.defaultFormatter": "mfeckies.handlebars-formatter"
}

[rwjblue]

Not sure exactly what will need to be done, but wanted to point folks to some changes coming to @glimmer/syntax (which is what prettier uses, right?):

• https://github.com/glimmerjs/glimmer-vm/pull/983
• https://github.com/wycats/handlebars.js/pull/1584

This will significantly aid codemod utilities (ember-template-recast / prettier) by ensuring that the resulting AST includes things that had whitespace control.

[velrest]

Hey, just wanted to ask if this is intended behavior: Before:

<CardComponent
  @name="test"
  @size="l"
  @onclick={{true}}
  @color="default"
  as |card|
>
  <card.header>
    content
  </card.header>
</CardComponent>

After:

<CardComponent
  @name="test"
  @size="l"
  @onclick={{true}}
  @color="default" as |card|
>
  <card.header>
    content
  </card.header>
</CardComponent>

The as |x| is always moved to the same line as the last component param. IMO this is not as readable as a new line but if this is intended im okay with it.

Also all new lines added for readability are stripped out. Is this also intended?

[jgwhite]

@velrest could I confirm which version of Prettier you’re using?

@chadian you’re probably the most familiar with intended Prettier behavior at this point. Could you check the above?

[jgwhite]

We’re still looking for an adventurous contributor to take on {{! prettier-ignore }} support.

[velrest]

@jgwhite im using prettier version 1.18.0.

[velrest]

I just ran prettier with glimmer through a fairly large codebase and found some weird cases:

Only closing tags on new line Before

<UkButton
  @disabled={{or (if protectedPage (not warningRead)) protectedSystemPage}}
>

After:

<UkButton
  @disabled={{or (if protectedPage (not warningRead)) protectedSystemPage
  }}
>

Inserts newline into HTML attribute value if hbs variable is used Before:

<span
  class="delete pointer show-delete-block-{{elementId}} uk-invisible uk-flex uk-flex-middle uk-flex-center"
>

After:

<span
  class="delete pointer show-delete-block-
    {{elementId}}
    uk-invisible uk-flex uk-flex-middle uk-flex-center"
>

Without the {{elementId}} the line is not split.

Another example: Before:

<button
  class="uk-button uk-button-{{if @selected "primary" "default"}} {{unless @icon "round-button"}}"
>

After:

<button
  class="uk-button uk-button-
    {{if @selected "primary" "default"}}

    {{unless @icon "round-button"}}"
>

Newline inserted for every hbs statement Before:

<span>
  ({{inc currentCardIndex}}/{{cards.length}})
</span>

After:

<span>
  (
  {{inc currentCardIndex}}
  /
  {{cards.length}}
  )
</span>

It seems like the second and third issue are caused by enforcing that each hbs statement is on a new line.

[jgwhite]

@velrest this is great stuff to build test cases around!

Could you try Prettier 1.19.1 and see if any of this improves?

[velrest]

@jgwhite So the "Only closing tags on new line" and the "Newline inserted for every hbs statement" cases are resolved with prettier@1.19.1.

The "Inserts newline into HTML attribute value if hbs variable is used" is still a issue.

All examples in the same order as before Before:

<UkButton @disabled={{or (if protectedPage (not warningRead)) protectedSystemPage}} />
<span class="delete pointer show-delete-block-{{elementId}} uk-invisible uk-flex uk-flex-middle uk-flex-center" ></span>
<button class="uk-button uk-button-{{if @selected "primary" "default"}} {{unless @icon "round-button"}}" ></button>
<span>({{inc currentCardIndex}}/{{cards.length}})</span>

After:

<UkButton
  @disabled={{or (if protectedPage (not warningRead)) protectedSystemPage}}
/>
<span
  class="delete pointer show-delete-block-
    {{elementId}}
     uk-invisible uk-flex uk-flex-middle uk-flex-center"
></span>
<button
  class="uk-button uk-button-
    {{if @selected "primary" "default"}}

    {{unless @icon "round-button"}}"
></button>
<span>
  ({{inc currentCardIndex}}/{{cards.length}})
</span>