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

Maintain relative order of attributes and comments within an ElementNode #1266

Merged
merged 3 commits into from
Feb 12, 2021

Conversation

charlespierce
Copy link
Contributor

@charlespierce charlespierce commented Feb 11, 2021

Info

Currently, if you have a comment in among the attributes of an angle bracket component, the Printer will output that comment at the end of the list (even if it was originally in a different location). This causes problems if you have a bare attribute (without a value), as the resulting template is now invalid:

<Foo {{!-- This is a comment --}} attribute></Foo>

Becomes

<Foo attribute {{!-- This is a comment --}}></Foo>

Which then fails to parse with the error:

Using a Handlebars comment when in the `afterAttributeName` state is not supported

Changes

  • Updated the printer to order all of the attributes, modifiers, and comments of an element by their original position in the source, preserving the user's ordering when printing the AST.
  • Updated the parser to not error when a Handlebars comment follows a valueless attribute.

Tested

  • Added test to confirm that the ElementNode ordering is preserved.
  • Added test to confirm that a comment is valid after a valueless attribute.
  • All tests pass.

@rwjblue
Copy link
Member

rwjblue commented Feb 11, 2021

There are two major issues here:

  1. when we print out the AST we end up discarding the original ordering of attributes, modifiers, and comments
  2. the error mentioned should be removed, and tests added to ensure that it works properly

@charlespierce charlespierce changed the title Add test case showing comment reordering in component attribute Maintain relative order of attributes and comments within an ElementNode Feb 11, 2021
@charlespierce charlespierce marked this pull request as ready for review February 11, 2021 23:19
@@ -53,3 +55,30 @@ export function escapeText(text: string): string {
}
return text;
}

export function sortByLoc(a: ASTv1.Node, b: ASTv1.Node): -1 | 0 | 1 {
Copy link
Member

Choose a reason for hiding this comment

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

Can you re-export this from the top level @glimmer/syntax index file? That will ultimately allow me to remove the original code this is copied from (over in ember-template-recast).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Worth noting, the detection of whether a Node is invisible or not is slightly different from the original in ember-template-recast, not sure if that will impact your ability to re-use it there.

Copy link
Member

Choose a reason for hiding this comment

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

Ya, it definitely will but ember-template-recast has not yet updated to the latest @glimmer/syntax yet. When it does, it'll need to deal with this new loc format as well, so it should be good to go.

Copy link
Contributor

@dcyriller dcyriller Oct 27, 2021

Choose a reason for hiding this comment

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

@rwjblue rwjblue added the bug label Feb 12, 2021
@rwjblue rwjblue merged commit 1a54cba into glimmerjs:master Feb 12, 2021
rwjblue added a commit to emberjs/ember.js that referenced this pull request May 3, 2021
rwjblue added a commit to emberjs/ember.js that referenced this pull request Jun 3, 2021
dcyriller added a commit to lifeart/ember-template-recast that referenced this pull request Oct 27, 2021
dcyriller added a commit to lifeart/ember-template-recast that referenced this pull request Oct 27, 2021
dcyriller added a commit to ember-template-lint/ember-template-recast that referenced this pull request Oct 28, 2021
dcyriller added a commit to ember-template-lint/ember-template-recast that referenced this pull request Oct 28, 2021
dcyriller added a commit to ember-template-lint/ember-template-recast that referenced this pull request Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants