New major version -- what do we want to change

[blutorange]

Regarding #4, now might be a good a time as any to start a new major release and get rid of some of the legacy weight. Especially now that the number of users is still rather low. Perhaps we can start by brainstorming what we would like to change.

As a general guideline, I also don't think we need to make a plugin that can be customized to suit for everybody's code style. For one thing, there are other plugins such as @trivago/prettier-plugin-sort-imports.

And, more importantly, prettier itself is already an opinionated code formatter that prescribes a certain code style. For reference, here's is their rationale: https://prettier.io/docs/en/option-philosophy.html

They only want options that are required for some purpose, such as to support external tools. But no options that are just people's opinions on how the code style should look like.

Also: people who need a very particular code style won't be using prettier, and in extension, they also wouldn't use this plugin. I beliebe prettier plugins should keep at least somewhat to the spirit of prettier. Having a single plugin that by now probably has accumulated more options than the program for which the plugin is meant feels a bit strange. Having many options also makes tests and maintenance a pain, since you now have to test various combinations of options, or get lots of bug reports if you don't.

Let's review the options we've got

• importOrderBuiltinModulesToTop - We wanted to remove this in the next major release, it always puts builtins at the top.
• importOrderParserPlugins - Needs to stay, this is required for the plugin to work correctly
• importOrderSortSpecifiers - Any reason not to remove this and have it enabled always?
• importOrderSeparation - Are people really that picky about having new lines? I also work with Java + Eclipse and it just groups imports and separates them by new lines, and nobody in our team has an issue with this. I'd just remove this and have it
• importOrderGroupNamespaceSpecifiers - I'm honestly not exactly sure what this does. I also don't see any tests where this is set to true?
• importOrderCaseInsensitive - This is an option I could image we might want to keep. Any opinions? If we don't, what would be the value for this?
• importOrder - Other than the technical importOrderParserPlugins, I feel like this should be the main option of this plugin, if not the only one.
  • The default for this option is [], which I feel is a pretty bad default. It just sorts everything alphabetically, including relative imports etc.
  • The other thing I don't like about this option is that it requires you to specify each @scope manually. Adding a new dependency now requires you to review and sometimes change the prettier config for this plugin.
  • What I'd suggest is that by default, the plugin just groups all @scope/... imports, builtin imports, and relative imports automatically.
  • Another additional idea would be to get rid the regexp approach and replace it with a more restricted approach that only allows the order of the various groups to be changed, something like importOrder: ["builtins", "unscoped", "@ui", "relatives", "@typescript", "scoped" ]

For the new options proposed by #4

Two more options are definitely too much. If anything, since some people might prefer separate type imports, I'd propose a single option importOrderMergeTypeImports with two values separate and mixed. Default being mixed.

• mixed combines all type and value imports import {foo, type bar} from "a"
• separete actively splits all type and value imports, ensuring a consistent style. So it would change import {foo, type bar} from "a" into import {foo} from "a"; import type {bar} from "a"``

Although I'd also be fine with not having this option and just defaulting to mixed.

What we need to check

We should take a look at common, default ESLint / TypeScript ESLint options and check if they conflict with the way this plugins formats code.

[fbartho]

Overall, I support the project of slimming down the options.

importOrderBuiltinModulesToTop

I say we drop this option & do this always.

importOrderSeparation

I say we drop this option, and let people insert "" in importOrder to express where they want gaps. (There may be a bug in how that works now, at least I struggled with that).

importOrderCaseInsensitive

I say we drop this option, and just default it to always-insensitive. I have yet to meet someone who believed capital letters shouldn't be sorted adjacent to their lowercased forms.

importOrderSortSpecifiers

I say we drop this option, and default to always sorting. I can't see a runtime-reason not to do this, and it's in the name of the plugin.

importOrderParserPlugins

Are we sure we have to keep this option? Can we detect decorators from tsconfig::experimentalDecorators? Can we detect jsx from file-extension? Can't we add typescript if a tsconfig is present?

importOrder

I definitely think we should keep this valuable option.

Why: I specifically think it's valuable because many codebases need to highlight clusters of internal packages and treat them like a special cluster of 3rd party packages. This is what my team members commented on when I added this plugin -- this was a convention before, that had to be manually enforced. Prettier doing that for us was a win. If we lost this option our package-imports would be sandwiched with 80% of 3rd party imports above, and 20% of 3rd party imports below, followed by local imports. #awkward

Change Default: I think we should default to builtins, 3rd-party, followed by local files.

I don't think we should auto-group by scope-packages, I worry about the default behavior for newlines-between groups. Lots of random 3rd party modules come from scopes now, so I don't want scattered newlines in my imports.

importOrderGroupNamespaceSpecifiers

I say we drop this option, because as far as I can tell, you can't have multiple namespace specifiers in a single import expression: import * as K, * as L from "./foo"; // illegal I'm not sure what it used to do.

Behavior change: group & sort type-imports separate from value imports

Related to importOrderGroupNamespaceSpecifiers : for #20 -- I say we should group type-imports and value imports within a single ImportDeclaration import { type A, type B, c, d } -- and we should sort these groups. This was done in the codemod from @IanVS -- but he put type imports last. I think they should be first!

import {
  type A,
  B,
  type C,
  D,

(Case I'm worried about based on current behavior, we sort by identifier if I read the code correctly)

importOrderMergeDuplicateImports importOrderMergeTypeImportsIntoRegular

Combine these two into importOrderMergeImports with 3 values: "mixed" | "separate" | "off" with the default of "mixed" -- only if the above behavior change of grouping type-imports for independent sorting is applied.
-- I think the fact that in leaves us confused on how to merge comments has me worried.

EDIT: added more about importOrder & grouping by scope

[blutorange]

You did a great job summarizing this. 👍

importOrderParserPlugins

Are we sure we have to keep this option? Can we detect decorators from tsconfig::experimentalDecorators? Can we detect jsx from file-extension? Can't we add typescript if a tsconfig is present?

Since I didn't write the original plugin, I'm not sure about the edge cases. Though in general we need to keep in mind that auto detection can fail for unusual use cases (e.g. if somebody runs prettier programatically?). I'd say we apply auto detection (if that's possible) when importOrderParserPlugins is not specified, but keep it for when auto detection fails. Unless somebody knows of a way that we're sure will never fail. I'm fine with a non-formatting related option.

importOrderCaseInsensitive

I think you're right, case-insensitive is probably the preferred approach here. But just to be clear, do you suggest that all type imports should come before value imports (import {type Z, A } from "a") ?

This was done in the codemod from @IanVS -- but he put type imports last. I think they should be first!

Personally I'd also put type import first if I have to do it manually.

If we lost this option our package-imports would be sandwiched with 80% of 3rd party imports above, and 20% of 3rd party imports below, followed by local imports.

Just for reference, could you post a sample of how your imports look like, and how your importOrder settings looks like?

I don't think we should auto-group by scope-packages, I worry about the default behavior for newlines-between groups. Lots of random 3rd party modules come from scopes now, so I don't want scattered newlines in my imports.

Yeah, more and more packages are moving towards scopes and it's not too hard to image that sooner or later most if not all packages could be scoped.That was just a wild suggestion. I'm more concerned with a sensible default anyways, and importOrder: ["builtins", "thirdParty", "local"] sounds good.

[IanVS]

@fbartho @blutorange thanks for the detailed proposals here. Overall, I think we're all in agreement, with perhaps a few small details to work out.

importOrderSeparation
I say we drop this option, and let people insert "" in importOrder to express where they want gaps. (There may be a bug in how that works now, at least I struggled with that).

What problems have you been having? Maybe you can open a new issue for it?

Personally I'd also put type import first if I have to do it manually.

Just curious, why do you both prefer that? My rationale for putting value imports first was that value imports feel more "important", and they're also often times shorter (especially including the type notation), and it just feels more balanced to have shorter things first. /shrug. I don't feel strongly about it, but I do prefer value before types. Curious to hear your thoughts.

importOrder

I agree with updating the default to sort builtins, third-party, then relative imports, but keeping it as an option. I also agree with not treating scoped packages as special. Personally, in my project I use @ for aliases as well (wish I had picked $, but oh well), and I need a custom importOrder rule to keep those separated from third party scoped packages. I'd also like to maintain the regex ability. For instance, I want to ensure that my (non-side-effect) .css files are sorted after my javascript files. So, my full importOrder looks like this:

importOrder: [
    '^(@api|@assets/|@components/|@hooks/|@util/|@dn-types/|@test-setup/|@global/)(.*)$',
    '^(?!.*[.]css$)[./].*$',
    '.css$',
],

importOrderParserPlugins

Let's keep this as an escape hatch, as @blutorange suggests.

importOrderMergeTypeImports

I think this is fine to have as a new option. But, we'll need to keep in mind that separate will only apply to explicit type imports, right? Right now, it's possible to mix value and type imports without specifying which ones are types. I imagine that we won't do anything to change those, even if the option is separate, right?

importOrderGroupNamespaceSpecifiers

Here's where this was added: trivago/prettier-plugin-sort-imports#105. I don't see a ton of value in it, personally, and would be okay with removing it.

[fbartho]

What problems have you been having? Maybe you can open a new issue for it?
I remember now that it was actually caused by #17 so I have no objections to assuming it should work, and defaulting to that.

type-import-grouping

Personally I'd also put type import first if I have to do it manually.
Just curious, why do you both prefer that? My rationale for putting value imports first was that value imports feel more "important", and they're also often times shorter (especially including the type notation), and it just feels more balanced to have shorter things first. /shrug. I don't feel strongly about it, but I do prefer value before types. Curious to hear your thoughts.

It's definitely subjective, but it feels like the very top things are the least important, because you're going up to check on them from reading the code and scanning back up to the references. Putting the type Foo statements first allows you to scan up first to the runtime imports, and treat the types more as ambient? -- Super subjective and I won't fight you for the default choice. Consistency is king, so I'm fine whichever way we end up.

importOrderGroupNamespaceSpecifiers

Oh Ew! This complicates our alphabetical sorting, and I'd vote delete the functionality!

importOrderParserPlugins

Unanimous: keep but try to add auto-detection so it's an escape hatch

[blutorange]

importOrderMergeTypeImports

I think this is fine to have as a new option. But, we'll need to keep in mind that separate will only apply to explicit type imports, right? Right now, it's possible to mix value and type imports without specifying which ones are types. I imagine that we won't do anything to change those, even if the option is separate, right?

Yes, I consider resolving imported modules and applying TS specific logic to check whether an import is a type or a value to be beyond the scope of what this plugin can and should do.

Personally I'd also put type import first if I have to do it manually.

Just curious, why do you both prefer that? My rationale for putting value imports first was that value imports feel more "important", and they're also often times shorter (especially including the type notation), and it just feels more balanced to have shorter things first. /shrug. I don't feel strongly about it, but I do prefer value before types. Curious to hear your thoughts.

That's really just my gut feeling. I don't look much at imports in general, but perhaps I'm just not working on large enough projects? As I've said, personally I only want a reasonable default (canonical) behavior I can apply to all of our projects, with the same configuration (if any) for all.

If I had to give a reason, I'd put all import type { ... } from "a" before value imports, like this

And perhaps by the same token, I'd use the order import { type A, type Y, B, X } from "a".

But come to think of it, if somebody uses import { type A, type Y, B, X } from "a", that looks a lot like they really just want to separate type and value imports. If somebody wants imports to be mixed, then perhaps we should just go for import { type A, B, X, type Y } from "a".

So:

// mode: separate
import type { A, Y } from "a"
import { B, X } from "a"

// mode: mixed
import { type A, B, X, type Y } from "a"

Other than that, I'd say we also consider how it's being done in various projects and what seems to be most common

Several widely-used packages I looked at use eslint-plugin-simple-import-sort (https://github.com/mongodb/node-mongodb-native/blob/main/package.json#L60, prisma). That plugin has only one option: Custom grouping (which corresponds to our importOrder) If you check out their github, they also have a rationale and explanation there. Perhaps we could take some ideas from them as well.

eslint-plugin-simple-import-sort just ignores the type specifier for sorting, but in case of a tie, it puts import type {} from "a" before import {} from "a" . This would correspond to our ignore setting.

importOrderCaseInsensitive

Not sure how this plugin handles it, but as mentioned by the eslint-plugin-simple-import-sort, if we sort case-insensitively, we need to make sure that we break ties by falling back to case-sensitive sorting (or we end up with non-deterministic formatting).

E.g.

import { a } from "someTHING"; 
import { a } from "SOMEthing"; 

[IanVS]

Let's discuss the ordering of the mixed option somewhere else, we can feel it out both ways and make a decision later.

Good point about breaking ties with imports that differ only by capitalization. That's something we should check. I've opened #23 so we don't forget.

[blutorange]

Let's discuss the ordering of the mixed option somewhere else, we can feel it out both ways and make a decision later.

Yes, let's how it goes. The main purpose of this issue was to find a direction for where we want to take this plugin, and it seems we all agree on that.

[fbartho]

• I've filed Feature: Smarter auto-detection of parser-plugins #24, Feature: group type-imports when sorting Identifiers #25 for further discussion.
• I've also added a milestone for stuff I think we should address in this breaking release.

How do we want to proceed? I'm happy to charge ahead on this.

Proposal: Can we get #19 and #20 to merge on the understanding that we're not going to ship it that way, and then we build from there?

[fbartho]

Hey @blutorange @IanVS I still want to make progress on what we talked about! — Can we get #19 & #20 to merge, as additive options — that we’ll delete pretty quickly?

I’d be happy to roll forwards with the other refactors to prepare a major release as we discussed up-thread?

[IanVS]

Sounds good. I'll finish reviewing your existing PRs as soon as I can.

[IanVS]

I wanted to remove importOrderCombineTypeAndValueImports and always enable it, but I think it's not possible without checking that the user's TypeScript version is at least 4.5. I also don't want to try to detect the user's TypeScript version, for a few reasons:

• It's tough to do reliably.
• It would likely require reading a package.json file from the filesystem, which would impact perf.
• Upgrading TypeScript would trigger a mass reformat to combine type and value imports, which is a little unexpected.

So instead, I'm going to experiment with enabling the option by default, with the possibility for the user to disable it if they're using an older version of TypeScript. Flow has supported the syntax forever, so it's not a problem there at least.

[fbartho]

We could offer a typescript max version override to enable this and future version downgrades?

[IanVS]

Hmmm, that's an interesting thought. Can you talk a bit more about how you might envision that working, @fbartho? If I understand you correctly, we'd add a setting like importOrderTSVersion that the user could set to whatever their version is, and we might use it to determine the kind of syntax to generate. For now, maybe we just use it for this case of importOrderCombineTypeAndValueImports, but there may be other features in the future as well that depend on the TS version.

I think I like it. It should be clear to the user what the value should be set to (they don't have to make a decision), and still allows us to be opinionated about the formatting. I guess the issue would be what do to if the user doesn't supply a version. The safest thing would be to assume it's 4.4 or below, and let them opt into new features by specifying their version. That would avoid unintentional breaking changes in the future as well as new features are potentially released for newer versions.

[fbartho]

If I understand you correctly, we'd add a setting like importOrderTSVersion that the user could set to whatever their version is, and we might use it to determine the kind of syntax to generate. For now, maybe we just use it for this case of importOrderCombineTypeAndValueImports, but there may be other features in the future as well that depend on the TS version.

@IanVS You exactly understood what I was suggesting. For now this would be limited to this one edge case.

I would suggest we say “absence means we’ll generate the most modern output”. It’s not like we have a habit or need to generally downgrade typescript-dependent syntax, so we may never use it again, but this way it’s not a flag that says “include option 45”.

By defaulting to the most modern output, we can avoid having most users need to specify this.

[IanVS]

I see your point, and I considered defaulting to most modern, but I foresee that causing two problems:

1. Anyone using a version of TypeScript older than 4.5 will have a broken codebase after using this plugin, unless they change the setting. That's not a great first-time experience, and many users will not spend the time to figure out what they need to do to fix it.
2. If we want to add new features for more modern versions in the future, it would require a major release because users would be automatically opted-in to the new behavior.

By defaulting to the least-modern, we avoid those issues, at the cost of not merging type and value imports for TypeScript users unless they provide a version of 4.5 or above. In that situation, their code at least still works, and since we'll have very few options in the plugin by then, it'll be easy to discover and opt-in to that feature.

[fbartho]

You have me pretty convinced @IanVS -- I defer to your decision.

For what it's worth, auto-detecting typescript version is as simple as:

function loadTargetTSVersion({ importOrderTypescriptVersion }: PrettierOptions) {
   if (importOrderTypescriptVersion != null) {
       return importOrderTypescriptVersion;
   }
   try {
       const ts = require("type" + "script"); 
       // choose your favorite way to defeat a bundler. 
       // Alternatively `import("typescript")` would work if async is acceptable.

      // ts.version '4.9.5'
      // ts.versionMajorMinor '4.9'
      return ts.version;
   } catch (e) {
      // Typescript is not installed
      return "1.0.0";
   }
}

And if we auto-detect, then only people that use this plugin in a "multi-typescript" environment, or people with typescript only available as a peerDependency will have this problem, but if TS is only a peerDependency, they wouldn't trip over the weird syntax here right?…

[IanVS]

That's an interesting approach to detecting the TS version, and I think it would work in most cases. However, it could return bad results if someone has multiple versions of typescript installed in their node_modules (either due to monorepo setup, or a transitive dependency that brings in typescript). The version that our plugin requires could differ from the version the user executes to actually check or transform their code. I was chatting with one of the TS maintainers, and he suggested looking into using https://github.com/sindresorhus/resolve-from, and I guess we'd resolve from cwd. We can try looking into that, but I worry about it being fragile and/or hurting performance. Making the setting explicit also gives folks a way to opt-out of type and value combining, in case it turns out that people really don't like that.

I guess the best of both worlds would be to auto-detect the version and still allow folks the ability to override the setting with their config.

[IanVS]

I think the last thing remaining that we talked about for 4.0 is a way to automatically detect which parser plugins should be used (#24). It seems like the current defaults are working for most users, or at least we haven't heard from users that it's a problem. I'm tempted to release 4.0 without that feature, and perhaps consider it together with automatically detecting the typescript version as we also discussed above.

If that's the case, I think there are two more issues that we can look into and try to solve as part of 4.0:

• Line break removed when importOrderCombineTypeAndValueImports is used #54
• Comments on same line as import are moved down #9

Lastly, I think we should look through upstream issues/PRs/releases/discussions to see if there's anything we want to pull in or address here. Let me know if you think there's anything else I'm forgetting about before we can release 4.0.

[fbartho]

I think we can release 4.0 without #24 and address that later — if it’s a big pain point. It’s only a pain for new plugin adopters though, so we may not get many complaints.

---

#9 and #54 seem related, and are indeed important.

I think what’s going on is that comments in certain positions are considered “attached” to the import syntax node, and we either render that attachment wrong (injecting a new line) or reformat the comment, and eat a new line.

[IanVS]

If you have a little time, would you like to look into that and see if you can fix it? I can take a pass through the upstream repo to see if there's anything else important for us.

[fbartho]

Yeah, added a bunch of annoyingly bugged test cases, now to fix them all

[fbartho]

Now that #71 is merged, do we have any more outstanding work that we wanted to get done before 4.0.0? (note: trivial cleanup in #74)

I don't think we to advertise prettier-3 support yet since that's still in alpha, and also, see: #75 (comment)

[IanVS]

I'm going to release another prerelease after reviewing your PRs, then the only other thing I planned to do was take a look through the upstream issues and PRs to see if there's anything we can quickly tackle.

[IanVS]

I almost forgot we wanted to change the default importOrder. I've created an issue and tagged it with the 4.0 milestone.

[IanVS]

Some upstream issues/prs we might want to consider:

• Sort both <script> and <script setup> for vue sfc, fix #218 trivago/prettier-plugin-sort-imports#219
• Formatting fails with error "'export declare' must be followed by an ambient declaration" in d.ts file trivago/prettier-plugin-sort-imports#203
  • (could we add dts: true to the default?)
• support absolute path imports as local imports (ts/jsconfig.json) trivago/prettier-plugin-sort-imports#162
  • Could be interesting to explore as a follow-on to 4.0
• Don't do anything if importOrder == [] trivago/prettier-plugin-sort-imports#181
  • Is an interesting one. Should we add a way to disable the plugin?
• SyntaxError: Adjacent JSX elements must be wrapped in an enclosing tag trivago/prettier-plugin-sort-imports#199
  • Maybe avoid adding the jsx plugin when vue is used?

[fbartho]

I don’t think we should do:

• support absolute path imports as local imports (ts/jsconfig.json) trivago/prettier-plugin-sort-imports#162 - because detecting if a path is a local-import at babel-parser time is… difficult without touching the filesystem or trying to resolve it. Unless you say that paths with file-extensions should be treated as local imports?
• Don't do anything if importOrder == [] trivago/prettier-plugin-sort-imports#181 — couldn’t they change the parser plugins using the overrides and disable the plugin that way? Or is that an option that can’t be overridden per file?

I don’t think we’re subject to trivago/prettier-plugin-sort-imports#203 — is that something we’re seeing? (Worth adding a test-case maybe?)

No opinion on

• Sort both <script> and <script setup> for vue sfc, fix #218 trivago/prettier-plugin-sort-imports#219 — I don’t use vue! I’m not sure what effect this would have on our parser’s code.
• re: SyntaxError: Adjacent JSX elements must be wrapped in an enclosing tag trivago/prettier-plugin-sort-imports#199 I had to disable jsx for Parser Error with the JSX parser plugin & TypeScript old-style assertions <SomeType>foo #78 so maybe we need a richer parser-plugin configuration system :(

[IanVS]

Unless you say that paths with file-extensions should be treated as local imports?

No, I don't think that's the way to do it, but we could potentially use the paths option from tsconfig/jsconfig. I don't think it's something we should address now, but personally I'd love to find a solution to this, because currently I have to duplicate some config between tsconfig and the prettier config to keep things sorted correctly.

couldn’t they change the parser plugins using the overrides and disable the plugin that way?

I didn't confirm yet, but I think that would be more likely to just cause an error to be thrown than to disable the plugin. And if they're using normal javascript, no plugins are needed to process the code, so there'd be no change. We could consider adding a importOrderDisable option to make it explicit.

maybe we need a richer parser-plugin configuration system :(

Perhaps. Or maybe we just need to do something like Vite does, and use overrides to only enable jsx for .tsx and .jsx files by default, and let the user specify it explicitly in their parser plugins option if they need it in other files. We could even do something similar for the typescript plugin, and only enable it in .ts and .tsx files, rather than everything. Though I haven't tried this out to see if it's really possible to do.

[IanVS]

Alright, I think we've done what we want to do for the 4.0 release. I'll close out this issue, and let the beta cook for a few days while before releasing 4.0 stable. Thanks for the excellent discussion and planning here in this issue!