add support for ember template tags · Pull Request #227 · IanVS/prettier-plugin-sort-imports

ghost · 2025-08-01T01:08:06Z

Closes #225.

Let's add support for ember's single-file component format (gts + gjs). This is largely just a port of trivago/prettier-plugin-sort-imports#377, so all credit really goes to the folks involved with that one.

That said, I'm still happy to make whatever changes needed to help this land here. Thanks again for the great plugin / considering this addition!

Closes #225. Let's add support for ember's single-file component format (gts + gjs). Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Co-authored-by: Robbie Wagner <robbie.wagner@hashicorp.com> Co-authored-by: Robbie Wagner <rwwagner90@gmail.com>

IanVS

This seems relatively clean and straightforward. I had a few, mostly minor, comments, but I think this looks good to move forward. I'm still not 100% certain on the use of parse-imports-exports and why that is required. I suppose it is because @babel/traverse cannot process .gjs/.gts files to extract import statements in the same way that we do for other files, is that your understanding as well?

@fbartho do you have any thoughts on this PR?

IanVS · 2025-08-01T12:39:07Z

+        if (!collection) return;
+
+        for (let [, info] of Object.entries({ ...collection })) {
+            for (let pos of info) {


Nit: I prefer using const for variables that are not intended to be changed, rather than let.

IanVS · 2025-08-01T12:41:56Z

+| Angular                | ✅ Everything   | Supported through `importOrderParserPlugins` API                                            |
+| Vue                    | ✅ Everything   | SFCs only, peer dependency `@vue/compiler-sfc` is required                                  |
+| Astro                  | 🧪 Experimental | Some Astro syntax may cause trouble, please open an issue                                   |
+| Ember                  | 🧪 Experimental | peer dependencies `prettier-plugin-ember-template-tag` and `parse-imports-exports` required |


I appreciate being conservative here and not requiring parse-imports-exports for non-Ember projects, but on the other hand I also kind of hate peer dependencies and this feels like an odd one to ask users to install manually. I'm all for keeping the project size small, but especially since it sounds like we might be able to use this utility library for other syntaxes as well, how about we just add it to our dependencies?

I tend to disagree @IanVS -- aren't we using the babel parser?

I don't know why we need an arbitrary source-code expression parser. I'm actually a little confused as to why this is necessary at all, actually. Is that because babel doesn't parse ember files?

~~As an example of what I'm worried about: parse-import-exports doesn't support Import Assertions/Attributes, while~~ babel would, I assume.
UPDATE: 2025-08-04 turns out that parse-import-exports they do support Import Attributes, and I was mistaken by reading the examples in their README.

Languages where we manipulate the babel nodes don't have this issue. This actually makes me think we shouldn't use parse-import-exports at all, actually!

Aside: We're might have a bug around that (when merging import nodes), but mostly we should be fine with support for those and/or I'm not sure there's a current use-case for assertions where merging nodes is relevant!

Is that because babel doesn't parse ember files?

Yeah, I believe it won't be able to parse the <template> blocks in particular. https://github.com/ember-tooling/prettier-plugin-ember-template-tag actually strips these out before running the code through the typescript babel parser: https://github.com/ember-tooling/prettier-plugin-ember-template-tag/blob/main/src/parse/index.ts#L100, but that logic is fairly complex to duplicate here.

It sounds like this is probably pretty far from being able to land if we want to avoid the library, so I'll go ahead and decline this for now.

Aside: We're might have a bug around that (when merging import nodes), but mostly we should be fine with support for those and/or I'm not sure there's a current use-case for assertions where merging nodes is relevant!

Ah, good catch on them not supporting import assertions, though that format is now deprecated syntax and "import attributes" are the standard: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import/with.

What kind of bug do you think we have with assertions / attributes? Maybe you can open an issue?

Nice catch on that being the deprecated concept. I vaguely remember hearing that, but it hasn’t been top of mind.

In the hypothetical merger scenario of something using with import-attributes, I think we’re arbitrarily picking one of the nodes to merge the other into (presumably the earliest node becomes the parent).

I’m not sure what we should do if they have import attributes:

Prevent the merge if they use attributes

Abort the merge if they don’t have identical attributes

Abort the merge if they have incompatible attributes.

Merge the attributes

I only know how to implement 1 & 2. No idea how to implement 3 & 4.

I’m kind of of the mindset of either doing nothing until somebody reports an issue (I have no idea who actually uses this feature, and how), or implementing one of 1 or 2.

Further question: Is it legal to merge something without attributes into an expression that has some attributes? — If that’s not legal, then we likely have a bug.

Filed a placeholder ticket here: #229

@fbartho by the way, why do you say "parse-import-exports doesn't support Import Assertions"? I played around with it a bit in the trivago repo (since they merged a very similar PR), and both assertions and attributes seem to work fine.

I read their README and they had a lot of examples of supported expressions, but assertions and attributes weren't documented there.

Further reading says they just added support as a feature 6 months ago -- literally the latest commit in the repo was a fix!

So that's absolutely my bad. I should have qualified my statement or otherwise explained how I came to that conclusion.

I still think manipulating the babel node-graph is much more wise than parsing it ourselves, but I retract my comment about this library not supporting that feature.

IanVS · 2025-08-01T12:49:08Z

Let's change this filename to ppsi.spec.ts so that it's not included in the snapshot.

IanVS · 2025-08-01T12:50:37Z

+import component from '@ui/hello';
+import fourLevelRelativePath from '../../../../fourLevelRelativePath';
+import * as a from 'a';
+import type RouterService from '@ember/routing/router-service';


let's add a few more styles like import type {T} from... and import a, type {T}, etc. Just for good measure.

IanVS · 2025-08-01T12:50:59Z

+import otherthing from '@core/otherthing';
+import abc from '@core/abc';
+import twoLevelRelativePath from '../../twoLevelRelativePath';
+import component from '@ui/hello';


Can we add a namespace import somewhere in here? import * as ....

IanVS · 2025-08-01T16:11:54Z

Oh also since I forgot to say it, thanks a ton for the PR, @geneukum!

ghost · 2025-08-01T22:11:15Z

+    ingest(importsExports.typeNamespaceImports);
+
+    let output = preprocessor(justImports, options);
+    let result = output + code;


Just noticed - this behaves a bit differently from the currently supported languages.

It looks like those sort imports relative to existing ones, where this just appends them all at the top of the file.

I think this is probably problematic if, for example, there's a license header at the top. This would move the imports above it, which is probably not what folks would want.

Let's see if we can fix this.

It looks like those sort imports relative to existing ones, where this just appends them all at the top of the file.

I think this is probably problematic if, for example, there's a license header at the top. This would move the imports above it, which is probably not what folks would want.

Full expected behavior documented in our README https://github.com/IanVS/prettier-plugin-sort-imports?tab=readme-ov-file#comments (included here for convenience):

If you have one or more comments at the top of the file, we will keep them at the top.

Comments on lines after the final import statement will not be moved. (Runtime-code between imports will be moved below all the imports).

In general, if you place a comment on the same line as an Import Declaration or *Specifier, we will keep it attached to that same specifier if it moves around.

Other comments are preserved, and are generally considered "leading" comments for the subsequent Import Declaration or *Specifier.

I would indeed expect the above behavior to be followed for Ember files as well, because as you said -- different behavior "is probably not what folks want".

I took a crack at this trying to use prettier-plugin-ember-template-tag to parse the file first, before doing our own ast traversal to extract import statements. However, the parse method is async, and preprocess has to be sync. I think that if the project exposed their own preprocess function (https://github.com/ember-tooling/prettier-plugin-ember-template-tag/blob/0ceae8900654e583435960da449ab9c7e6139cd3/src/parse/preprocess.ts#L94), we could use that to extract the code we need, but unfortunately it's not exported. The only other thing I think we can try is to duplicate much (all?) of the logic from that file. Which, it's not horrible, but definitely a bummer.

Closes #225. Adds support for ember's single-file component format (gts + gjs). This iterates upon #227, with significant changes. Now, we will preprocess the source code in a similar way to `prettier-plugin-ember-template-tag`, replacing `<template>` tags (using https://github.com/embroider-build/content-tag) with placeholder comments of the correct length. Then we'll parse that code, extract and sort import nodes, and substitute the new nodes into the _original_ code string. ~This does end up adding `content-tag` for all users, but it's a very small utility library so it shouldn't hurt anything.~ It's now an optional peer dependency, and must be installed manually for users of Ember template tags. --------- Co-authored-by: Geordan Neukum <gneukum1@gmail.com> Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Co-authored-by: Robbie Wagner <robbie.wagner@hashicorp.com> Co-authored-by: Robbie Wagner <rwwagner90@gmail.com>

IanVS reviewed Aug 1, 2025

View reviewed changes

ghost commented Aug 1, 2025

View reviewed changes

ghost closed this Aug 1, 2025

ghost deleted the add-glimmer-js-ts-support branch August 1, 2025 23:41

IanVS mentioned this pull request Aug 4, 2025

add support for ember template tags #231

Merged

This pull request was closed.

Conversation

ghost commented Aug 1, 2025

Uh oh!

IanVS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbartho Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fbartho Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanVS commented Aug 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fbartho Aug 1, 2025 •

edited

Loading

fbartho Aug 4, 2025 •

edited

Loading