-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Comments
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) Making those scenarios nice would bea blocker for me |
Also, what's the config for folks who run everything through eslint? |
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
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.
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 😅.
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:
module.exports = {
plugins: ['ember-template-lint-plugin-prettier'],
extends: ['octane', 'ember-template-lint-plugin-prettier:recommended'],
}; |
Well, before I adopt prettier in templates, i'd want the printWidth respected.
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. |
Tldr, requirement for me is:
|
@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 |
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.
{{!-- input --}}
<Component @date="dateFormat:YYYY-MM-DD hh\:mm\:ss" />
{{!-- output --}}
<Component @date="dateFormat:YYYY-MM-DD hh\\:mm\:ss" />
{{!-- 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}} |
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. I'm good with prettier for templates being added by default.. but only when it's ready. |
@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.
Nice to have
|
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> |
@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 |
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.
@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. |
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.
I'm personally okay with the existing formatting. The current behaviour is to put component + args on the same line until the |
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) |
Sure, you can use a |
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 |
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. |
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 |
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:
devDependencies
..template-lintrc.js
to include the Prettier plugin:Open issues for Glimmer syntax, which may need to be fixed before:
[handlebars] Named blocks can't be self closing prettier/prettier#11900(fixed by v2.5.1)Correctly identify which backslashes are removed by glimmer prettier/prettier#12302 Prettier should not re-escape a backslash in a string literal prettier/prettier#11401(fixed by v2.6.0)The text was updated successfully, but these errors were encountered: