rfcs
rfcs copied to clipboard
emberjs
Add Prettier for Handlebars to default blueprints
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: https://github.com/adopted-ember-addons/program-guidelines/pull/20#issue-1063949729
The following changes to default blueprints would be required:
- Add ember-template-lint-plugin-prettier to
devDependencies. - Change
.template-lintrc.jsto 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'], }; - 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:
- ~~https://github.com/prettier/prettier/pull/11900~~ (fixed by v2.5.1)
- ~~https://github.com/prettier/prettier/pull/12302 https://github.com/prettier/prettier/pull/11401~~ (fixed by v2.6.0)
- https://github.com/prettier/prettier/issues/10163
- https://github.com/prettier/prettier/issues/10847
- https://github.com/prettier/prettier/issues/10178
- https://github.com/prettier/prettier/pull/10759
Why num 2? I've been using it with single quotes app wide and it works great.
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.
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
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'],
};
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.
Example of obviously worse behavior: https://prettier.io/playground/#N4Igxg9gdgLgprEAuEAeAJgSwG4AIwA2AhgM4kC8A5ADpS64BGA5gLQMECucjEATunF4sATD36CWvOOlzwAHjBZMpCWvQBm0RQwgEZABzksArLn0BPEbmWYZTIvpYAWXCQAWRdBADua1x69vJV4ic1kQqBJMGExoFiICAlxNMA4SInY4P15MKFYXekLCvz9gYEx1XAABLHTMmT96Slz4ELAY7DgWWoyCaUpcRtxm2EEidpw4GjpcAF9ZoaKi0vLKgAo+XDW4AEdqqIAvbkoCJkoASi3d6uwiHKJYYeJeJinz86HKTVgWU+StNi6dDTIpfAEAWxk30U4OkmA44JBcwWUEoAD4VlsYG5MCQAHSaCC4ACMuGEl3mJRmZQAxH0YFsSHwGYslmt1JgCK1BjMlkU1l4WNjckwSDy+XzSWtIOD9Bx4MM3II3rgAMyXVmgiDoYGa+jvcW4Ui4AA+BFxMBNlN5uFpcHGblw5pIDONJqgCKtKIlZQ94OtRTKAHp7WA3AHbcAg-Tragg1hsBibbQyuZMHA9LhonBwUQs-BwQwth53EbyOyIETSeTLjAIFRzHdKPMQAAaEAQfQxaAkZCgO68HwABTuCF7KAS3lCvfbDDaAGs4DAAMpEWEAGVycGQ6gSJDgs4XS+X+nGIuQMF4XHbOYY0gE6HXDyYHCIrwAYnxczAYnlkCAiHlCA2xANwYHBAgAHUcXgEhTzAOBlzHaIcGicx-zAMgQNyfdeBgIcQiYXMdz3A8QAAKxIORlxFPoAEUOAgeASIIfd21PXhcP-JhzXBWFeBA-QclgSDbGxZAAA4AAZ2MHfdIJCfR-yEuBcM6ECdkY+ACM7ccAJIFgoDgaRpBAqRNMwKQCLfYikF3ViyP3cFMAvK9HNouAGKY7c7NI9sYAyUT0HEpBhH8kJORFABhCA+KIf9VOMEC0jgAAVDJx3stiQGwLgAEkoAEWBlzAHIuwAQUK5cYHMPoWP3eYgA
And with printWidth=40
https://prettier.io/playground/#N4Igxg9gdgLgprEAuEAeAJgSwG4AIwA2AhgM4kC8A5ADpS64BGA5gLQMECucjEATunF4sATD36CWvOOlzwAHjBZMpCWvQBm0RQwgEZABzksArLn0BPEbmWYZTIvpYAWXCQAWRdBADua1x69vJV4ic1kQqBJMGExoFiICAlxNMA4SInY4P15MKFYXekLCvz9gYEx1XAABLHTMmT96Slz4ELAY7DgWWoyCaUpcRtxm2EEidpw4Gig-AF9ZoaKi0vLKgAo+XDW4AEdqqIAvbkoCJkoASi3d6uwiHKJYYeJeJinz86HKTVgWU+StNi6dDTIpfAEAWxk30U4OkmA44OmjXmtEoAD4VlsYG5MCQAHSaCC4ACMuGElxRdEGVLKAGI+jAtiQ+IzFks1upMARWtSlny1l4WNjckwSLy+UtSWz+ZBwfoOPBhm5BG9xRL6ABmaWFS7apoQdDA2i0D5U+iXUi4AA+BFxMCtlKKdLg4zcuFtJEZlqtUARDoWZqdwF94MdhTKAHoXWA3GHIwzKagI1hsBjA0MyuZMHA9LhonBwUQ8-BwQwth53LgiOQORAiaTyZcYBAqOY7pR5iAADQgCD6GLQEjIUB3Xg+AAKdwQQ5QCW8oSHPYYbQA1nAYABlIiwgAyuTgyHUCRIcCXq-XG-04xFyBgvC4PYLDGkAnQO4eTA4RFeADE+IWYBiPJkBAIgFQgbsQDcGBwQIAB1HF4BIK8wDgDdp2iHBonMECwDISDchPXgYHHEImELQ9j1PEAACsSDkDcRT6ABFDgIHgSiCBPHsr14IiQKYW1wVhXhIP0HJYDg2xsWQJwAAYeLHE84JCfQQPEuAiM6SCdjY+BSL7GdQJIFgoDgaRpEgqRdMwKRSO-CikCPLjqJPcFMFve9XKYuBWPYg8nKonsYAyKT0BkpBhGCkIuRFABhCBhKIEDNOMSC0jgAAVDIZ2c7iQGwLgAEkoAEWANzAHJ+wAQVKjcYHMPpOJPeYgA
Tldr, requirement for me is:
- Class wrapping
- formatting nested helper invocations based on readability, rather than printWidth
@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.
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.
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.
We found two issues this the prettier printer when we ran it on our app.
- 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" />
- 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
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.
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 :-\
@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
- [ ] ...
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
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>
@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
- 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 https://github.com/prettier/prettier/issues/9984. That issue was closed. Accordingly to the issue it is "actually unsupported syntax". This seems to be relevant as well: https://github.com/ember-template-lint/ember-template-lint/issues/787
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 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.
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.
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)
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
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.
I removed https://github.com/prettier/prettier/pull/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.
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.
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.
Updated the initial post as the changes to .eslintrc.js are not needed anymore for latest Ember CLI versions thanks to https://github.com/ember-cli/ember-cli/pull/10062.