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

Use insertFinalNewline option from editorconfig for Handlebars / Ember files #10759

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

dcyriller
Copy link
Collaborator

@dcyriller dcyriller commented Apr 22, 2021

Description

By default, Prettier will remove the final newline from Handlebars / Ember files. Why? Because otherwise it would add an extra DOM text node and in most cases this would be unnecessary. This is the recommended practise in the Ember ecosystem (source).

In this PR I propose a small adjustement to this default behavior. Prettier would read insertFinalNewline option from editorconfig (for Handelbars / Ember files only). When the option is set to true, Prettier would add a final newline. When set to false, the final newline would be removed.

This PR is still a draft. Indeed I'd like this option to be read only from editorconfig (not user configurable). But I don't know how to do that and I would have to spend some time on it. So, I prefer to discuss the PR first not to engage too much work in case you don't want to go down that path. May you let me know if you are okay with the direction it is taking?

Links

Checklist

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@dcyriller dcyriller marked this pull request as draft April 22, 2021 13:32
@dcyriller dcyriller changed the title WIP: Use insertFinalNewline option from editorconfig for Handlebars / Ember files Use insertFinalNewline option from editorconfig for Handlebars / Ember files Apr 22, 2021
@thorn0
Copy link
Member

thorn0 commented Apr 22, 2021

It's difficult to agree with the Ember team's reasoning about doing the newline stripping on the editor's side.

From ember-cli/ember-cli#3440 (comment):

That could break cases where a component's white-space is pre

Isn't this an edge case that happens really rarely though? Besides, it looks like the problem can be solved by putting a comment at the end of such a template to control how many significant trailing blank lines it should have.

content ...

{{! The two newlines before this comment should be preserved. The final newline after this comment shouldn't matter. }}

There are great points in ember-cli/ember-cli#3738

@fisker
Copy link
Member

fisker commented Apr 22, 2021

I alway like the idea repect editorconfig, not just handlebars. Every editor I use take precedence over editor settings, why shouldn't we?

@thorn0
Copy link
Member

thorn0 commented Apr 22, 2021

@fisker So are you okay with adding an option controlled only by EditorConfig?

@fisker
Copy link
Member

fisker commented Apr 22, 2021

Yes.

@thorn0
Copy link
Member

thorn0 commented Apr 22, 2021

That's an interesting solution to the "How do we add an option without adding an option?" problem. Need to think more about it.

@dcyriller
Copy link
Collaborator Author

Thank you for your feedbacks.

That's an interesting solution to the "How do we add an option without adding an option?" problem. Need to think more about it.

I'll try to think about it as well. If you have any pointer, it is welcome :)

@tylerbecks
Copy link

tylerbecks commented Jul 1, 2021

Any updates on this? I'm being affected and would love to see this merge!
This is the problem I'm having: ember-template-lint/ember-template-lint-plugin-prettier#100

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

Successfully merging this pull request may close these issues.

None yet

4 participants