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

[QUEST] Glimmer #1

Closed
jgwhite opened this issue Jun 19, 2019 · 135 comments
Closed

[QUEST] Glimmer #1

jgwhite opened this issue Jun 19, 2019 · 135 comments

Comments

@jgwhite
Copy link
Owner

@jgwhite jgwhite commented Jun 19, 2019

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
Copy link
Collaborator

@rwjblue rwjblue commented Jun 19, 2019

RE: the entity escaping, we really need to avoid the escaping all together (see prettier#4533 (comment) for more details)

@GavinJoyce
Copy link
Collaborator

@GavinJoyce GavinJoyce commented Jun 19, 2019

I'm working on fixing the HTML entities here: prettier#6234

@tchak
Copy link
Collaborator

@tchak tchak commented Jun 19, 2019

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

@GavinJoyce
Copy link
Collaborator

@GavinJoyce GavinJoyce commented Jun 20, 2019

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 prettier#4908

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

@tchak
Copy link
Collaborator

@tchak tchak commented Jun 20, 2019

I just linked the main issue to this one

@dcyriller

This comment has been hidden.

@Alonski
Copy link

@Alonski Alonski commented Jun 20, 2019

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

@jgwhite
Copy link
Owner Author

@jgwhite jgwhite commented Jun 20, 2019

@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
Copy link
Collaborator

@GavinJoyce GavinJoyce commented Jun 20, 2019

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
Copy link
Collaborator

@dcyriller dcyriller commented Jun 20, 2019

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
Copy link
Collaborator

@GavinJoyce GavinJoyce commented Jun 21, 2019

@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
Copy link
Owner Author

@jgwhite jgwhite commented Jun 21, 2019

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

@efx
Copy link

@efx efx commented Jun 21, 2019

@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
Copy link
Owner Author

@jgwhite jgwhite commented Jun 21, 2019

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

@dcyriller
Copy link
Collaborator

@dcyriller dcyriller commented Jun 27, 2019

I opened a PR to add trailing newline to hbs files: prettier#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
Copy link
Owner Author

@jgwhite jgwhite commented Jun 27, 2019

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

@jgwhite
Copy link
Owner Author

@jgwhite jgwhite commented Jun 27, 2019

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
Copy link

@Alonski Alonski commented Jun 27, 2019

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

@laurmurclar
Copy link

@laurmurclar laurmurclar commented Jun 27, 2019

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
Copy link
Owner Author

@jgwhite jgwhite commented Jun 27, 2019

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
Copy link
Collaborator

@dcyriller dcyriller commented Jun 27, 2019

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
Copy link
Owner Author

@jgwhite jgwhite commented Jun 29, 2019

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
Copy link
Collaborator

@dcyriller dcyriller commented Jun 29, 2019

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
Copy link

@samselikoff samselikoff commented Jun 29, 2019

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
Copy link

@laurmurclar laurmurclar commented Jul 9, 2019

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
Copy link
Owner Author

@jgwhite jgwhite commented Jul 10, 2019

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

@bartocc
Copy link

@bartocc bartocc commented Jun 5, 2020

With prettier 2.0.5, we ran into an issue when trying to format the following index.hbs file:

Start a <span>télé-</span>consultation
  • output with prettier --parser html index.hbs:
Start a <span>télé-</span>consultation
{{!-- template untouched --}}
  • output with prettier --parser glimmer index.hbs:
Start a
<span>
  télé-
</span>
consultation
{{!-- a space will be rendered between `télé-` and `consultation`. --}}

I believe this is a bug because I don't see any reason why the glimmer parser should generate a different output from the html parse.

For the time being, our only workaround is to wrap the template's content in a div tag and disable prettier for this div

{{! prettier-ignore }}
<div>
  Start a <span>télé-</span>consultation
</div>

cc: @pbernery @MonsieurDart @pauljeannot @Noctambul

@stfnio
Copy link

@stfnio stfnio commented Jun 5, 2020

t being on a new line (from {{) also breaks highlighting in my VSCode.

I think that's the only thing that stops me of using "format on save".

@dcyriller
Copy link
Collaborator

@dcyriller dcyriller commented Jun 5, 2020

  1. We’ve fallen a little behind on keeping the checklist up-to-date, but take a look at reports such as #1 (comment) and see if you can submit a PR to Prettier that fixes it.

To follow-up on what @jgwhite wrote: an intermediate step can be to open an issue in Prettier repo. It is very easy to do thanks to https://prettier.io/playground where you can copy-paste your code and click the Report issue button. The support for Glimmer parser was recently added to the playground.

@dcyriller
Copy link
Collaborator

@dcyriller dcyriller commented Jun 5, 2020

@kangax
1/ For now, any comment that includes a mustache is formatted with {{!-- --}} and any comment not including a mustache with {{! }}. Thus, the formatted code remains correct regarding (handlebars documentation for comments). But your question was: why this formatting instead of respecting the user choice for one syntax or the other? I can't tell for sure because I don't have the whole history, but I think that the problem is that these two syntaxes have an exact same AST representation. So when we print them, we don't know if the user used the former or the latter syntax.

That said, I've not yet figured out why we have two different comment syntaxes in handlebars 😄. They have the same AST. The only difference I see between the two syntaxes is that the former supports including mustache. So, should we converge on formatting every comment to {{!-- --}}? EDIT: the topic is already discussed in this RFC

2/ At the moment, Prettier will always remove the final newline, see this snapshot (the snapshots are formatted to be human readable).

On that topic, Prettier complies with the choice made in ember-cli blueprints. Why removing the final newline? Because it inserts a trailing TextNode which is useless and sometimes can break the UI.

We could change that and try to respect the user's choice made through editorconfig's insert_final_newline option. To do so, we'd have to add support for insert_final_newline option in https://github.com/josephfrazier/editorconfig-to-prettier. I'm not sure if they'd be willing to add it though (usually reluctant to adding more options). If they're not ready to accept it, we'd be back to the choice made in ember-cli and if we want to change that choice, we could open a RFC.

3/ and 6/: I agree the formatting of these snippets doesn't look good. Should we open issues?

4/ and 7/ is the same issue. It was not formatted this way in previous versions, it is a regression (@nightire shows it a few comments above). We could open an issue to track the progress made on it.

5/ I think this is expected for html for now (so for handlebars as well). There is a discussion opened on this topic in https://github.com/prettier/prettier/issues/, 5246.

@EvgenyOrekhov

Are there plans to add support for hbs template tag?

In the javascript printer there is a embed hook. It could be used to format some handlebars code (embed in javascript code) with Glimmer printer. Should we open an issue to track that?

@mydea

Which is IMHO not ideal for Git versioning etc.

I agree.

Also, I opened prettier#8502

@Alonski
I reported this issue here: prettier#8504 :)

@EvgenyOrekhov
Copy link

@EvgenyOrekhov EvgenyOrekhov commented Jun 5, 2020

@dcyriller

Are there plans to add support for hbs template tag?

Should we open an issue to track that?

Yes, please!

@stfnio
Copy link

@stfnio stfnio commented Jun 8, 2020

Just came up with this weird result of formatting a file. Reproducible by having a string which starts with \n and continues on the next line. Could you add this to the list of knows issues?

@amk221
Copy link

@amk221 amk221 commented Jun 17, 2020

What's recommended for a situation like this:

{{! in }}
<FormField>Field Name<Required /></FormField>
{{! out }}
<FormField>
  Field Name
  <Required />
</FormField>
{{! desired }}
Field Name*
{{! actual }}
Field Name *

@amk221
Copy link

@amk221 amk221 commented Jun 18, 2020

{{! input }}
\{{my-component}}
{{! output }}
{{my-component}}

..this causes the my-component to be rendered, when it shouldn't.

@salomvary
Copy link

@salomvary salomvary commented Jun 19, 2020

In:

<!DOCTYPE html>
<html>
<body>
    {{{body}}}
</body>
</html>

Out:

{{{body}}}

Works as expected without <!DOCTYPE html>. See in playground.

@kangax
Copy link

@kangax kangax commented Jun 22, 2020

@salomvary I looked into the DOCTYPE issue and it seems that glimmer parser doesn't recognize it in the first place. It's not even in the AST so prettier can't process it. Looks like this is a known issue and isn't something prettier can fix until glimmer adds support for it — glimmerjs/glimmer-vm#870

@rwjblue
Copy link
Collaborator

@rwjblue rwjblue commented Jun 22, 2020

I had a PR for it, but ended up closing 🤔

@kangax
Copy link

@kangax kangax commented Jun 26, 2020

@bartocc @amk221 the incorrect linebreak in your examples is a known issue (and is probably one of the major ones since it affects rendering) — prettier#8527

@dcyriller I was looking more into 3rd issue in my #1 (comment) and realised that prettier is inconsistent in how it handles this. JSX does not break if it's 1 attribute with long value while both HTML and Glimmer printers do. Now I'm not sure what it should be, we can consider this feature not a bug :)

@jgwhite could you update the checklist for this ticket? looks like all of the issues listed are now fixed. Should we instead just link to bugs labeled with "lang:handlebars"?

@jgwhite
Copy link
Owner Author

@jgwhite jgwhite commented Jun 26, 2020

@jgwhite could you update the checklist for this ticket? looks like all of the issues listed are now fixed. Should we instead just link to bugs labeled with "lang:handlebars"?

@kangax that is a terrific idea, done 👍

@amk221
Copy link

@amk221 amk221 commented Dec 21, 2020

There's an issue with {{! prettier-ignore }}

If you want a file with no whitespace in it:

{{! prettier-ignore }}<div>{{@text}}</div>

Results in:

{{! prettier-ignore }}
<div>
  {{@text}}
</div>

What's the situation with thread now? Should we still be raising issues here, or do it on Prettier repo proper?

@nabeelz6
Copy link

@nabeelz6 nabeelz6 commented Dec 22, 2020

rwjblue commented on Jun 19, 2019
RE: the entity escaping, we really need to avoid the escaping all together (see prettier#4533 (comment) for more details)

GavinJoyce commented on Jun 19, 2019
I'm working on fixing the HTML entities here: prettier#6234

So while HTML entity names (e.g. &lt;) are not being converted since this fix, It seems that prettier via glimmer parser is still converting HTML entity numbers (decimals) via glimmer parser which doesn't seem right.

Before (see playground):

Are not modified characters: &lt; &gt; &amp;
Are modified characters: &#60; &#292; 

After:

Are not modified characters: &lt; &gt; &amp;
Are modified characters: < Ĥ

We have a fairly large codebase using HTML entity numbers and while converting some of the numbers won't break anything (except maybe some older browsers), it does break for things like &#60;. As a workaround, we could manually replace all instances with their equivalent names but it does feel like prettier shouldn't be messing with HTML entities period.

Should I add a bug for this in prettier repo? Btw, current glimmer behavior is also inconsistent with JSX parser (see prettier#678).

References:

@rwjblue
Copy link
Collaborator

@rwjblue rwjblue commented Dec 23, 2020

@nabeelz6 - Mind making a failing test case for glimmerjs/glimmer-vm for this? Off the top of my head, I don't know where those are being processed but a test case should help us get started.

@dcyriller
Copy link
Collaborator

@dcyriller dcyriller commented Dec 28, 2020

I opened a PR to have a whitespace-sensitive mode for Prettier. It could be a solution to the whitespace issue around span and other inline elements.

I ran the code against the ember-observer code base. Any feedbacks on how the templates are formatted is welcome in the PR: dcyriller/client#2.

@thorn0
Copy link

@thorn0 thorn0 commented Feb 8, 2021

So, Glimmer folks, now that we have --html-whitespace-sensitivity, does anything else still separate the Glimmer parser from being out of experimental?

@chrisrng
Copy link

@chrisrng chrisrng commented Feb 9, 2021

Agree ^ anyone know the maintainers to see if we can cut a release?

@efx
Copy link

@efx efx commented May 26, 2021

@thorn0 @dcyriller a huge thank you for landing Handlebars support!
I just started to read the official blog post goodies

@jgwhite
Copy link
Owner Author

@jgwhite jgwhite commented May 26, 2021

I’d say we can close this issue now 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet