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.

Sign up for GitHub

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

Jump to bottom

Handlebars: Fix incorrect classic component syntax formatting #8593

Merged
merged 9 commits into from Jun 24, 2020
Merged

Handlebars: Fix incorrect classic component syntax formatting #8593

merged 9 commits into from Jun 24, 2020
+171 −2

Conversation

mikoscz
Copy link
Contributor

@mikoscz mikoscz commented Jun 18, 2020 •
edited

Resolves:
#8584
jgwhite#1 (comment)
jgwhite#1 (comment)

  • I’ve added tests to confirm my change works.
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/handlebars/pr-8593.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

@mikoscz mikoscz changed the title WIP: Fix wrong indentation of classic component path WIP: Handlebars: Fix incorrect classic component syntax formatting Jun 18, 2020
@mikoscz mikoscz changed the title WIP: Handlebars: Fix incorrect classic component syntax formatting Handlebars: Fix incorrect classic component syntax formatting Jun 18, 2020
@mikoscz mikoscz force-pushed the fix-wrong-indentation-of-classic-component-path branch from 5674e08 to 6e8d7f4 Compare June 18, 2020 14:52
@mikoscz mikoscz force-pushed the fix-wrong-indentation-of-classic-component-path branch from 6e8d7f4 to a3a3486 Compare June 18, 2020 14:54
@fisker
Copy link
Member

fisker commented Jun 18, 2020 •
edited

@mikoscz Do you know how it broken in v2.0? Parser upgrade or something ?

alexander-akait
alexander-akait reviewed Jun 18, 2020
View reviewed changes
src/language-handlebars/printer-glimmer.js
@@ -649,6 +651,10 @@ function locationToOffset(source, line, column) {
}
}

function doesNotHaveHashParams(node) {
return node.hash.pairs.length === 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible when node doesn't have hash or hash.pairs properites?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to glimmer MustacheStatement interface, this is not possible.
https://github.com/glimmerjs/glimmer-vm/blob/cba8afd3e665087ce9aa1457181b1955cfee91f0/packages/%40glimmer/syntax/lib/types/nodes.ts#L92
https://github.com/glimmerjs/glimmer-vm/blob/cba8afd3e665087ce9aa1457181b1955cfee91f0/packages/%40glimmer/syntax/lib/types/nodes.ts#L226

@thorn0 thorn0 linked an issue Jun 18, 2020 that may be closed by this pull request
Prettier formats handlebars components incorrectly inside html tag #8584
Closed
@dcyriller
Copy link
Collaborator

Could you also add a test for:

<div>
  {{a-helper "that takes some arguments" this.anotherOne "why not" @aLastOneLongEnoughToBreak}}
</div>

@mikoscz
Copy link
Contributor Author

mikoscz commented Jun 19, 2020 •
edited

Could you also add a test for:

<div>
  {{a-helper "that takes some arguments" this.anotherOne "why not" @aLastOneLongEnoughToBreak}}
</div>

@dcyriller how it should behave?

  {{
    a-helper
    'that takes some arguments'
    this.anotherOne
    'why not'
    @aLastOneLongEnoughToBreak
  }}
  {{a-helper
    'that takes some arguments'
    this.anotherOne
    'why not'
    @aLastOneLongEnoughToBreak
  }}

If the second one I think that we can change the behaviour to:
"Break it only if the mustache statement does not have any params(both positional and named)"

@dcyriller
Copy link
Collaborator

@fisker I believe it's regression introduced in #7157

@mikoscz
Copy link
Contributor Author

mikoscz commented Jun 19, 2020

@dcyriller Do you think that we need something more to move forward with this one?

@dcyriller dcyriller mentioned this pull request Jun 22, 2020
Closed
@thorn0 thorn0 merged commit aa1088f into prettier:master Jun 24, 2020
@mikoscz mikoscz deleted the fix-wrong-indentation-of-classic-component-path branch June 25, 2020 08:44
@kangax kangax mentioned this pull request Jul 1, 2020
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ziw ziw ziw left review comments

@fisker fisker fisker approved these changes

@alexander-akait alexander-akait alexander-akait approved these changes

@dcyriller dcyriller dcyriller approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prettier formats handlebars components incorrectly inside html tag
6 participants
@mikoscz @fisker @dcyriller @ziw @alexander-akait @thorn0