IanVS / prettier-plugin-sort-imports

An opinionated but flexible prettier plugin to sort import statements
Apache License 2.0
951 stars 21 forks source link

A way to exclusively sort type-only imports #107

Closed DaveLo closed 1 year ago

DaveLo commented 1 year ago

Is your feature request related to a problem? I'm not sure if this is a bug or feature tbh, but if I have <TYPES> in the importOrder array my merged imports get split, which shouldn't happen in the version of TS I'm using (5.1.3) and makes the import lines more verbose.

I'm using prettier v2.8.8 and v4.0.2 of this lib.

Describe the solution you'd like It would be ideal if there was a way to exclusively sort type-only imports that wouldn't affect mixed imports.

Describe alternatives you've considered My current options seem to be either have type-only imports mixed into normal imports by the sorter, or have type imports always on a separate line even if there is an existing import statement.

Additional context Config:

{
  "importOrder": [
      "",
      "^@myorg/(.*)$",
      "",
      "^[./]",
      "",
      "<TYPES>"
  ],
  "importOrderTypescriptVersion": "5.1.3"
}

Simple Example:

// before
import { dbMiddleware, type DBKeys } from "@myorg/db";
import type { GetCustomerReq } from "@myorg/schema";

// after
import { dbMiddleware } from "@myorg/db";

import type { DBKeys } from "@myorg/db";
import type { GetCustomerReq } from "@myorg/schema";

// expected
import { dbMiddleware, type DBKeys } from "@myorg/db";

import type { GetCustomerReq } from "@myorg/schema";
IanVS commented 1 year ago

Thanks for bringing this up. I could see maybe adding a special word for <TYPE_ONLY> or something like that. I am about to go on vacation for a while, but feel free to submit a PR if you'd like to dig in, and I can take a look when I get back (or @fbartho may be able to while I'm gone).

fbartho commented 1 year ago

@DaveLo I see why you're frustrated by this.

The whole point of the <TYPES> feature was "all type imports should be in one place, because you know they cannot then have side-effects" -- so it's working as we intended it to (to be clear, I don't want this in my codebases, but I was convinced that others do!)

As @IanVS says, we could build a thing that works like you ask -- and we can collectively bike-shed how to spell it. If you want to submit a PR, I'd be happy to provide feedback and help you get it functioning, and then when @IanVS comes back, we can discuss about getting it merged & released.

DaveLo commented 1 year ago

I can take a crack at it!

fbartho commented 1 year ago

@DaveLo I'd like to encourage you to weigh whether your requested feature is worth the additional special word -- as this is likely to be pretty complex code to weave into the rest of the plugin, and will likely have a bunch of nuance that needs to be documented.

Specific problem cases that jump to mind include:

DaveLo commented 1 year ago

@fbartho I was thinking about that while reading through the codebase over the weekend. I almost think it more belongs as an option? I know you all have been working to limit down option complexity though.

To me it seems we would either want types to separate (current behavior) or only to separate type-only imports (proposed option). Making it something to opt-in on makes it easier to document the behavior while removing some of those edge cases.

Does that seem reasonable?

fbartho commented 1 year ago

This time last year, we had hit the point where we had too many options-flags for different behavior, and it was being a maintainability/complexity problem -- not to mention being overwhelming for new users. We noticed users struggling with too many options with the various upstreams/forks or alternatives of this plugin.

Removing various options flags simplified the code, and allowed us to more easily fix various long-standing bugs. This further brought us more in-line with Prettier's "Opinionated" configuration -- options only for the most critical needs. I think that we're in a much better place now, so I'm loathe to add a new option.

Here's a question for you @DaveLo: can you achieve your desired behavior without needing a new option flag?

{
  "importOrder": [
      "",
      "^@myorg/(.*)$",
      "<TYPES>^@myorg/(.*)$",
      "",
      "^[./]",
      "<TYPES>^[./]",
      "",
      "<TYPES>"
  ],
}

The gotcha of this approach is that you'll still have two imports from the same file. One will be runtime only, and then it will be followed by types-only import declaration.

fbartho commented 1 year ago

An approach I could be convinced would work would treat <TYPES> as some sort of fancy regex flag:

- "^@myorg/(.*)$",
- "<TYPES>^@myorg/(.*)$",
+ "(<TYPES>^@myorg/(.*)$)|(^@myorg/(.*)$)"

If this type of expression was supported by us, then you'd be able to express your importOrder without needing a new options-flag. (Fixing the gotcha of my proposal above).

DaveLo commented 1 year ago

I generally think that idea would be intriguing and allow a user to opt in to the level of complexity they need.

I don't think it meets my use case from a style perspective (explicit order based on type-only without affecting mixed order). That said, I'm also now pretty convinced that what I'm looking for is outside the scope of what this plugin is aiming for as it's a pretty niche use case and probably not worth the added complexity of either a new reserved keyword or flag.