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

Add Prettier for Handlebars to default blueprints #777

Open
jelhan opened this issue Nov 30, 2021 · 30 comments
Open

Add Prettier for Handlebars to default blueprints #777

jelhan opened this issue Nov 30, 2021 · 30 comments

Comments

@jelhan
Copy link
Contributor

jelhan commented Nov 30, 2021

I'm wondering if it is time to add Prettier for Handlebars to default blueprints.

This would be a follow-up to using Prettier for JavaScript files, which landed in #628. At the time we adopted Prettier for JavaScript files, Prettier for Handlebars wasn't stable yet. It is stable since Prettier v2.3, which was released in May this year.

@patocallaghan summarized the trade-offs very well in his proposal to use it for adopted-ember-addons: adopted-ember-addons/program-guidelines#20 (comment)

The following changes to default blueprints would be required:

  1. Add ember-template-lint-plugin-prettier to devDependencies.
  2. Change .template-lintrc.js to include the Prettier plugin:
    diff --git a/.template-lintrc.js b/.template-lintrc.js
    index f35f61c..0520e96 100644
    --- a/.template-lintrc.js
    +++ b/.template-lintrc.js
    @@ -1,5 +1,6 @@
    'use strict';
    
    module.exports = {
    -  extends: 'recommended',
    +  plugins: ['ember-template-lint-plugin-prettier'],
    +  extends: ['recommended', 'ember-template-lint-plugin-prettier:recommended'],
    };
  3. Configure Prettier to use double quotes for Handlebar templates only (if using Ember < 4.10):
    diff --git a/.prettierrc.js b/.prettierrc.js
    index 534e6d35aa..e5f7b6d1ee 100644
    --- a/.prettierrc.js
    +++ b/.prettierrc.js
    @@ -1,5 +1,12 @@
     'use strict';
     
     module.exports = {
    -  singleQuote: true,
    +  overrides: [
    +    {
    +      files: '*.{js,ts}',
    +      options: {
    +        singleQuote: true,
    +      },
    +    },
    +  ],
     };

Open issues for Glimmer syntax, which may need to be fixed before:

@knownasilya
Copy link
Contributor

knownasilya commented Nov 30, 2021

Why num 2? I've been using it with single quotes app wide and it works great.

@jelhan
Copy link
Contributor Author

jelhan commented Nov 30, 2021

Why num 2? I've been using it with single quotes app wide and it works great.

Our official documentation uses double quotes in Handlebar template files - even though it is using single quotes in JavaScript files. Also most of the applications and addons, I have seen, are following these convention. Changing this would cause unnecessary noise. Even if we want to alter the conventions anyways, I would prefer to not couple it with Prettier for Handlebars but have a separate RFC.

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Nov 30, 2021

How is prettier for tailwind users? (Class attribute wrapping)
And how does it handle formatted nested helper invocations? (Not inlining)

Making those scenarios nice would bea blocker for me

@NullVoxPopuli
Copy link
Contributor

Also, what's the config for folks who run everything through eslint?

@patocallaghan
Copy link

patocallaghan commented Dec 1, 2021

Thanks for opening @jelhan. Personally I wouldn't be against this. Having rolled it out over a large codebase the benefits of not having to think about template formatting is nice. Yes it's not perfect but neither is Prettier for JS and we've just accepted its choices there. Prettier for Handlebars is considered "stable" now and I think we should push for its adoption, if even to see what the blockers are or else we'll never see it get widespread usage.

@NullVoxPopuli re:your questions

How is prettier for tailwind users? (Class attribute wrapping)

So currently it will put all classes on a single line. We use Tailwind heavily and while the formatting here is not ideal we still decided to go with using Prettier for Handlebars as overall the positives outweighed the negatives. This is actually a contentious issue in the Prettier world in general. In the newly released Prettier 2.5 they've reverted some Tailwind specific changes for HTML formatting to just put all classes on one line until they can figure out something better.

And how does it handle formatted nested helper invocations?

I haven't seen any issues personally here but with regards these formatting tools we chose just to run with the choices Prettier made because once everything is up for debate you'll never align 😅.

Also, what's the config for folks who run everything through eslint?

We just followed @jelhan's awesome blog post which outlined how to configure ember-template-lint with prettier. Essentially it was just but this should also be part of the RFC:

yarn add -D prettier ember-template-lint-plugin-prettier
module.exports = {
  plugins: ['ember-template-lint-plugin-prettier'],
  extends: ['octane', 'ember-template-lint-plugin-prettier:recommended'],
 };

@NullVoxPopuli
Copy link
Contributor

it will put all classes on a single line.

Well, before I adopt prettier in templates, i'd want the printWidth respected.
For tailwind users, this is currently an unacceptable compromise, imo.

I haven't seen any issues personally here but with regards these formatting tools we chose just to run with the choices Prettier

I understand this is the prettier philosophy, but we still have a lot of say in how things are represented, since prettier support is still early.
For example, if i have 4 nested helper invocations, that better not be inlined to a single line. I know not of a single person who would think that's on improvement.

@NullVoxPopuli
Copy link
Contributor

Tldr, requirement for me is:

  • Class wrapping
  • formatting nested helper invocations based on readability, rather than printWidth

@patocallaghan
Copy link

patocallaghan commented Dec 1, 2021

@NullVoxPopuli Thanks for these, they give more concrete examples of what you'd like improved. Should we open issues against Prettier to generate discussion if they haven't already? Personally I'm fine with the behaviour of nested helpers obeying printWidth (maybe I'm nuts 😅) but I do agree the class on a single line at least respecting printWidth would be an improvement.

@scottmessinger
Copy link
Contributor

While Prettier could be improved in some specific ways (like those mentioned above), I think having it in the default blueprint is an enormous benefit and I think we should do it. I'll never go back to having to manually format HBS files and I would love for each new user to Ember to experience the simplicity of automatic formatting by default.

@ro0gr
Copy link

ro0gr commented Dec 1, 2021

At the end formatting is needed to improve experience for the source code reader, since we read more than write. So if it'd impove DX but degrade readability(for instance due to some basic issues mentioned by @NullVoxPopuli), I'd say it may be a bad default.

@kiwiupover
Copy link

kiwiupover commented Dec 1, 2021

We found two issues this the prettier printer when we ran it on our app.

  1. prettier added an extra \ to backslash in a component argument value.
{{!-- input --}}
<Component @date="dateFormat:YYYY-MM-DD hh\:mm\:ss" />

{{!-- output --}}
<Component @date="dateFormat:YYYY-MM-DD hh\\:mm\:ss" />
  1. Prettier didn't understand this older mustache syntax
{{!-- input --}}
{{#let importInput.[0] importInput.[1] importInput.[2] as |importRow|}}
  {{importRow.name}}
{{/let}}

{{!-- output --}}
{{#let importInput.0 importInput.1 importInput.2 as |importRow|}}
  {{importRow.name}}
{{/let}}

https://prettier.io/playground/#N4Igxg9gdgLgprEAuEAeAJgSwG4D4A6UABEagMIQC2ADtAjEQALoCG8AvPiK-AGIQAnSmyQBNcaIC0AWWmSAIvKIALZfnxJKldUgDOurkQD0BYkUIlgwAMQAbOA0w1BMAJJRqAVxgA6ANoADAC6RE60Am4e3v4AjCFhLu5evn4ATCEsukQAPgkRAEoQAO7ZAL6lFuYwVnkwhUU+UCyUcOWVVkb2MG1QqEZYeJUkIAA0IBDUMJjQusigLAICxQAKCwizKCy2RSwAnrNjAEYCLGAA1g4Ays1wADKYUHDIAGZbunBHJ+dX1KcPAObIGACTwfEBwSiHODodDQ24sKD-Twsf5wfhCNhTRHIEAsbwQUYgZQwSi2ADqykw8F0vzAcEu6ypOCpuxxYH0hIe7wiyxO-2ELzeYIAVroAB6XAH2ACKngg8EFtneY1+Am5OP+ticLQEhOoAgeMDJmHQMGUyAAHAEVUt3mSTtQcfq4NzsE8xgBHOXwXkTDa43SSR7Q6GEgRwL2YcO8lECpCvJVg96UTBAkFJqVwWXyp7xoVjGAsQ7G03mpCpAsnTBaxEULQsHEugCshM87wAKkWNgnlSBsKD3LDYJcwAbJgBBKDoS4wXb2RXvcpAA

@scottmessinger
Copy link
Contributor

At the end formatting is needed to improve experience for the source code reader, since we read more than write. So if it'd impove DX but degrade readability(for instance due to some basic issues mentioned by @NullVoxPopuli), I'd say it may be a bad default.

This feels rather reductive to me. For us, prettier has improved the readability, despite a few area where it could be better. Suggesting the lack of perfection causes a net decrease in readability isn't consistent with our experience with prettier.

If there are issues where prettier is changing the semantics of the templates (as @kiwiupover points out), that's definitely a reason to think twice about making prettier a default. We haven't run into many of those and they've all been solved by adding a prettier-ignore comment.

@NullVoxPopuli
Copy link
Contributor

you shouldn't have to disable lint/formatting when it's the default.
that's a bug, ya know?

I'm good with prettier for templates being added by default.. but only when it's ready.
It seems there are still bugs to fix atm :-\

@kiwiupover
Copy link

@NullVoxPopuli who will be responsible to the bugs to be worked out. Prettier for Glimmer is the responsibility of the ember community.

Can we make a list of things that need to be done before we can turn on prettier by default.

  • backslash escape
  • tailwindcss classes

Nice to have

  • New line for each argument passed to a component
  • ...

@NullVoxPopuli
Copy link
Contributor

Sounds good to me. I'm aware the ember community has to maintain this, but that's much different, imo, from using in the default blueprint

@void-mAlex
Copy link

likely people will also run into this gem:

{{#in-element @destinationElement }}

{{/in-element}}

results in

{{#in-element @destinationElement guid='%cursor:0%' nextSibling=null}}

{{/in-element}}

also

<label>
	<input
		class="flex flex-col {{if (and this.message this.test) "a" "b"}} {{if (and this.message this.test) "dsa asd" "a b"}} a"
		type="text"
		placeholder="test"
		value={{this.message}}
	/>
</label>

adds at the very least 1 extra new line

<label>
	<input
		class="flex flex-col
			{{if (and this.message this.test) "a" "b"}}

			{{if (and this.message this.test) "dsa asd" "a b"}}
			 a"
		type="text"
		placeholder="test"
		value={{this.message}}
	/>
</label>

@jelhan
Copy link
Contributor Author

jelhan commented Dec 2, 2021

  1. Prettier didn't understand this older mustache syntax
{{!-- input --}}
{{#let importInput.[0] importInput.[1] importInput.[2] as |importRow|}}
  {{importRow.name}}
{{/let}}

{{!-- output --}}
{{#let importInput.0 importInput.1 importInput.2 as |importRow|}}
  {{importRow.name}}
{{/let}}

@kiwiupover This was reported in prettier/prettier#9984. That issue was closed. Accordingly to the issue it is "actually unsupported syntax". This seems to be relevant as well: ember-template-lint/ember-template-lint#787

@jelhan
Copy link
Contributor Author

jelhan commented Dec 2, 2021

I added a list of open issues and pull requests in the first post. I hope this helps us tracking the issues, we may want to fix before adding Prettier for Handlebars to default blueprints.

New line for each argument passed to a component

@kiwiupover I haven't found a GitHub issue for this one in Prettier repository. Did I just overlooked it? Otherwise it would be great if you could add an issue for tracking.

@void-mAlex
Copy link

@void-mAlex Are you sure that you are using latest version of Prettier? I can not reproduce in the playground:

* https://prettier.io/playground/#N4Igxg9gdgLgprEAuExgGICWUC0cA2cAtgjAAQACAJnAM4zYCGD0AooSbGQL7cA6UAWgD02PB1K8QAGhAQADiyi1koRgCd1EAO4AFDQhUpG+bYwCeK2QCN1jMAGs4MAMqMSAGWxxkAMxO0cDZ2js4u8vbYAObIMOoArkEgxNZwVDRUHoxQUfGMUXAAYhDqRMwMOcggjPEwEDIgABYwRPgA6o2Y8LQRYHAuhl2YAG5d5lVgtFYg2IHqMLp2UWV+AUkAVrQAHi7RhACK8RDwq-iBshHqc1VR+JhEJOoN8urYMG2YVDCNyAAcAAwXLSBNp2eRVF50ODqYY+WQARyO8EWCiM1VoOCgcDSaQa6jgiMw+MW+RWSH8ZySgSImFiCSpezgh2OPnJa1kMEY1g+Xx+SAATBy7Jg7jkAMIQB6MKp0ACsDXigQAKlyjBTziBhokAJJQGiwFxgV6KACCepcMHMhFOgV4QA

* https://prettier.io/playground/#N4Igxg9gdgLgprEAuEAeANgQwEZ3QPgB0pCZUBLKABwFcZjTSwsBnFgXkJADN04APAAS8BAWkjpBwYOW6CAFJigATQTAAW5FgDoAtnDaYA5nDWad8FjACUgrpi52Q2LgF9XUmXMUqzWvQYsxqYa-pY2TspBgpgsyo72gi4g7jFcDPQwMACeVHCcIPD89CAZpFRYYHDqEOjKcABOBeHpJJkAbpjoNPnSoTr6hibuGQD0RFCoo1i4BCAANCAQVDDk0CzIoJgNDRAA7gAK2wgbKF17mNkbi9gNmGAA1nAwAMqY+gAylHDI3F0scBud0ezxeVHulCMyBgDR6izgulwynqyg+SiMNGCADEIA1dJgspDkCBMHQIAsQOoYLp0AB1TSWcFVF4ncirdps7LEsBsCmUAENGAHO5GfG-f6AkAAKxY-BekL4AEUaBB4OL0ADFuCGgLiUZ0ORdPoGhSqA1KDBaeRlBpkAAOAAMWt2ANpdyoxLNBka7R+iwAjir4MLlqcSSxRFA4HAURSGnBA+R48LjGKkH8NZKAbpyNDYVmFXBlaqfumJYsYDgrTb1MgAEwVu7kA1QIwAYQgRswxIMAFYKTQAQAVHCnDOakDtHoASRUCFeYHNKwAgioXjk+OqAe4gA

turns out I wasn't; at least the vs code extension wasn't properly updated. sorry for the noise, glad it was user error.

@patocallaghan
Copy link

patocallaghan commented Dec 3, 2021

So I'd be in agreement we need to fix some issues first, especially those that change the semantics of the template, before we could consider making this part of the default blueprint.

I still don't think the Tailwind classes formatting should be a blocker for adoption (saying this as a user of Tailwind myself). Formatting of Tailwind style classes is still a point of contention in the wider Prettier community and we should probably align with whatever happens there. When that will be finally agreed upon I'm not sure, but the existing behaviour we have in the Glimmer formatter when there are lots of classes is what has been adopted for Angular, React & Vue formatting.

New line for each argument passed to a component

I'm personally okay with the existing formatting. The current behaviour is to put component + args on the same line until the printWidth limit is hit, and then break onto new lines after that. This also aligns with how Prettier formats components + args in Angular, Vue + React.

@sandstrom
Copy link
Contributor

sandstrom commented Jan 2, 2022

I'm not a big fan of prettier for JS code (we use ESlint). Haven't tried out prettier for handlebars, so can't judge it.

One question though, will it be possible to run prettier only for handlebars, but not for the JS code?

I'm guessing that may be a use-case for some people? (it could be for us at least)

@nightire
Copy link

nightire commented Jan 2, 2022

One question though, will it be possible to run prettier only for handlebars, but not for the JS code?

Sure, you can use a .prettierignore file to exclude the .js files

@jelhan
Copy link
Contributor Author

jelhan commented Jan 2, 2022

One question though, will it be possible to run prettier only for handlebars, but not for the JS code?

Yes. That should be possible.

I assume that we will integrate Prettier as a plugin for Ember Template Lint to run it for handlebars files. Prettier for JavaScript is in the blueprints configured to run as an ESLint plugin. You should be able to remove the Prettier plugin for ESLint but keep the Prettier plugin for Ember Template Lint.

Ignoring files by extension using .prettierignore as suggested by @nightire might be another option. It may be helpful to prevent editors to run Prettier on files just because it is present as a dependency.

@jelhan
Copy link
Contributor Author

jelhan commented Feb 23, 2022

I removed prettier/prettier#11559 from the list above. Single quote option is working fine for Handlebars as far as I know. That draft merge request is about HTML parser.

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

Sounds to me like this is ready for an actual RFC. There may still be some issues to hammer out, but that’s part of the RFC process.

@jelhan
Copy link
Contributor Author

jelhan commented Jul 26, 2022

Sounds to me like this is ready for an actual RFC. There may still be some issues to hammer out, but that’s part of the RFC process.

I agree. The remaining open issues listed in first post are about coding style only. All reported bugs, which (may) changed semantics of code, have been fixed.

I just double checked open issues in Prettier repository itself. There are some additional open issues for Glimmer parser. But none of them affects usage in Ember applications. They seem to be only about Handlebar features not used in Glimmer templates.

I'm not sure how soon I will have time to write a RFC myself. If someone else is willing to take over, please do so.

@jelhan
Copy link
Contributor Author

jelhan commented Jan 23, 2023

Updated the initial post as the changes to .eslintrc.js are not needed anymore for latest Ember CLI versions thanks to ember-cli/ember-cli#10062.

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

No branches or pull requests