[Feature] Combine type and value imports

[IanVS]

Is your feature request related to a problem?
Not so much a problem, but an enhancement

Describe the solution you'd like
I'm not positive this fits in to the scope of this project, but thought I'd suggest it anyway. I recently created https://github.com/IanVS/type-import-codemod, which combines type and value imports into a single import statement, using a new feature of Typescript 4.5: import {foo, type Foo} from './foo';. If desired, I think it could be adapted or used within this prettier plugin.

If we did this, it would probably either need to be behind an option, or ideally, the version of typescript would be checked to find out whether it supports the new syntax, perhaps with an option to disable it. I normally want to avoid options, but since this feature isn't directly related to sorting imports, there should be a way to disable it, if we even include the functionality at all.

Describe alternatives you've considered
I could see creating a different prettier plugin just to combine those imports. That's honestly probably the better approach than trying to shim it into this one.

Additional context
I'm opening this issue to see if anyone else has thoughts about whether this would be useful.

[blutorange]

Hmm, I'd say it fits with the theme of normalizing imports (of which ordering is a part). Given the name of this plugin, it does sound like it should be another plugin. But we also need to consider that prettier doesn't have an API for these kind of AST changing plugins, so another plugin would have to parse the entire file again.

If we do this, I think we should not limit that to combining type and value imports, but combining duplicate imports in general, so that this would become one import.

import {a} from "./a";
import {type b} from "./a";
import {c} from "./a";

One complication I can think of is how to decide what is the same import?

// Are all these the same? What would users expect?
import ... from "./index";
import ... from "./index.js";
import ... from "../model/index";
import ... from "../model";
import ... from "../node_modules/orm/lib/index.js";
import ... from "orm";

I don't think we should try to normalize these paths since that can be specific to the OS and build system, but I feel users would expect at least some logical equivalent paths to be treated the same. How does type-import-codemod treat these cases?

Related:

• Sorting the imported symbols
• Non-deterministic order for type and value imports non deterministic order for "import type" trivago/prettier-plugin-sort-imports#116

And yes, I'd also like to stick to prettier's philosophy of keeping the number of options limited, one option for enabling or disabling this is OK imo, but not e.g. fine-tuned settings for how to order items. Personally, I care that imports have a defined order, I don't really care which one it is exactly (since I'll almost never write imports directly). In that sense, I feel this plugin already has too many options (though we shouldn't just remove those now).

[ajryan]

We'd like to use this syntax. Currently the plugin errors out on type inside the import braces. Would be nice to at least get a fix to treat type Xxxx as Xxxx and maintain the existing behavior otherwise.

[IanVS]

Currently the plugin errors out on type inside the import braces

I believe that means that either your typescript version or prettier version is not high enough. Would you mind opening an issue with the details of what you're seeing, and what versions you are using?

[IanVS]

I don't think we should try to normalize these paths since that can be specific to the OS and build system, but I feel users would expect at least some logical equivalent paths to be treated the same. How does type-import-codemod treat these cases?

Sorry, I missed this question earlier. Currently, the codemod only looks at the string literal value, it does not try to do any normalizing at all, since that seemed to be the safest approach.

If we do this, I think we should not limit that to combining type and value imports, but combining duplicate imports in general,

Maybe. Although, it's also pretty easy to prevent that situation using an auto-fixable eslint rule: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-duplicates.md.

But, it's easy enough to support that here too, I think.

[fbartho]

@blutorange I don't think we should be responsible for normalizing imports that are spelled differently:

import ... from "./index";
import ... from "./index.js";
import ... from "../model/index";
import ... from "../model";
import ... from "../node_modules/orm/lib/index.js";
import ... from "orm";

I do however think we should remove duplicate imports for things that are spelled identically!

I'm not an expert at AST modification, but it feels like a comparatively straightforward (not simple, exactly) AST modification "take children of this node A, and add them as children of node B, then finally delete node A (and then chain to our sorting mutation)". As I understand it, that should be about as complicated as "sort all the children of this node"

I may take a look at this operation to get familiar with the codebase

[blutorange]

@fbartho Yes, I don't think implementing this should be too hard. Feel free to take a look if you want to. You can start here https://github.com/IanVS/prettier-plugin-sort-imports/blob/main/src/utils/get-sorted-nodes.ts where it builds the finalNodes.

https://astexplorer.net/ is pretty helpful if you want to visualize the AST.

Some things to consider that also need appropriate tests

• Each node has a list of comments. If you delete an import node, you'll also have to move those comments to the new node
  • 
• How to combine different types of imports
  • Named and default imports: import { foo } from "a" and import bar from "a" need to be combined to import bar, {foo} from "a";
  • Side-effect and named imports: import { foo } from "a" and import "a". Could in principle be combined to import { foo } from "a", but we have to preserve the relative order of side-effect imports. So I think the easiest way to go about here is to ignore side-effect imports when combining imports.
  • Mixed type and value imports: import type { foo } from "a" and import { type bar, baz } from "a" can only be combined to import { type foo, type bar, baz } from "a" -- you need to remove the importKind from the ImportDeclaration
    • 

[fbartho]

1. I was aware of the comments, but was concerned about if simply merging the comments would make sense. Particularly for "inline" comments. I suspect that inline comments might lose their position.
2. I was initially planning not to convert import type { Foo } into import { type Foo because I could see arguments that this would make less verbose code turn into more verbose code. (Imagine 10 type imports, the plugin shouldn't be introducing more characters into the code)

[fbartho]

I realize now that the headliner feature on this ticket was actually to combine type & value imports -- for some reason my brain was stuck on just the classic duplicate imports problem. -- I've implemented that in #19

I'm happy to add an additional feature now to combine type & value imports.

[IanVS]

👍 To be clear, I'm not proposing converting from import type { Foo } to import { type Foo }, and that's not what my codemod does either. It only combines multiple imports together, if one is a type import and the other is a value import, of the same module. It may still introduce more characters into the code, but it would reduce the number of import statements. I'm not totally certain it belongs in this project, which is why I opened an issue for discussion.

[fbartho]

@IanVS well I did that first thing in #19, and the second thing in #20 (which builds upon #19). So I guess now we get to decide what to keep! haha

[fbartho]

I definitely think #19 belongs in this project, and #20 is growing on me -- definitely could see a codebase that rejects it, but I probably would turn it on for our monorepo.