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

Allow enforcement of import-specifiers #1787

Closed
manuth opened this issue May 29, 2020 · 20 comments · Fixed by #3043
Closed

Allow enforcement of import-specifiers #1787

manuth opened this issue May 29, 2020 · 20 comments · Fixed by #3043

Comments

@manuth
Copy link
Contributor

manuth commented May 29, 2020

Imports can have multiple specifiers (objects which are being imported).
Would be awesome if there would be a way to enforce alphabetization there, too:

Example:

import { isNull, isNaN } from "util"; // reports an error: "isNaN" should appear before "isNull"
@ljharb
Copy link
Member

ljharb commented May 29, 2020

Related to #1577, #1670.

@andrewbranch
Copy link

While eslint’s included sort-imports rule covers this, I came here looking for an alternative because that one doesn’t account for TypeScript type-only imports (and I don’t know enough about eslint internals to know whether it could, or whether they would accept such an addition even if it can). TypeScript’s own convention is to sort type-only import specifiers last:

import { x, y, z, type A, type B, type C } from "./mod";

But I haven’t found an eslint rule capable of enforcing this style in my brief search so far. I had thought I might find it in this plugin, since it does recognize import type and import typeof.

@ljharb
Copy link
Member

ljharb commented Dec 3, 2022

Does the typescript-eslint plugin have a sort-imports rule that covers it?

@andrewbranch
Copy link

No, I couldn’t find anything related in typescript-eslint.

@ljharb
Copy link
Member

ljharb commented Dec 5, 2022

@bradzacher do you think this would make sense in the TS eslint plugin?

@bradzacher
Copy link
Contributor

I think that it makes more sense in this repo so we don't "compete" on rules.
"for everything related to imports go here" is more user-friendly than "for sorting imports go here and for everything else go here".

I've been trying to draw the line of "if you're encoding TS-specific semantics about imports in a rule, typescript-eslint, otherwise, import-js"

Which is why I've added/proposed things over here instead of in our repo.

@ljharb
Copy link
Member

ljharb commented Dec 5, 2022

Makes sense in general; since sort-imports is a core rule already it seemed a reasonable suggestion :-)

@bradzacher
Copy link
Contributor

Yeah we have done it in the past with no-duplicate-imports - we added an extension for it because it's a base rule, then we realised that ofc there's import/no-duplicates which already existed and supported what users wanted.

We're removing our extension in the next major and directing people towards your rule instead.

So this would be the same case - instead of adding an extension rule to add support for type imports and competing, we'll just want a rule here that can freely support TS, JS and flow for the whole community!

@andrewbranch
Copy link

andrewbranch commented Dec 6, 2022

I think we (the TypeScript team) would be pretty interested in having an option for this. When we migrated our codebase to modules, we enabled the core sort-imports rule but noticed that our own auto-imports sorting logic doesn’t play nicely with it. I just wrapped up some work to make sure we can detect and conform to however sort-imports is configured, but if we start using any type-only import specifiers, our rules will again disagree with sort-imports, and I’m less willing to give up grouping of type-only imports. (cc @jakebailey)

@basicdays
Copy link

The other thing I noticed about eslint's base sort-imports rule is that it only handles named imports if it's all on the same line. In many cases I have instances that I rolled a number of nested modules into one top-level module that re-exports everything. This allows importing a bunch of things from one convenient module. Autoformatters will then line-break them. It would be nice if this rule could also handle ordering multi-line named imports too.

Example:

import {
    b,
    a,
} from './module';

gets sorted to ->

import {
    a,
    b,
} from './module';

@manuth
Copy link
Contributor Author

manuth commented May 5, 2023

For the time being, you might want to use @typescript-eslint/tslint in combination with tslint which does provide such a rule.

@ljharb
Copy link
Member

ljharb commented May 5, 2023

tslint has been deprecated since 2019, so i'm not sure that's a good recommendation, unless this is just a tslint-like eslint-based replacement?

@basicdays it's very unfortunate that eslint won't make changes to stylistic rules anymore, or else that would be a perfect place for that modification to be made.

Does anyone have a concrete suggestion for adding an option that would govern sorting of named imports within curly braces, whether single or multi-line? Note that with multiline, comment attachment becomes quite tricky wrt autofix.

@manuth
Copy link
Contributor Author

manuth commented May 5, 2023

It does use the actual tslint under the hood but it's integrated in eslint

@ljharb
Copy link
Member

ljharb commented May 5, 2023

oof, then I would strongly avoid it, since the tool's been deprecated (and afaik, unmaintained) for 4 years.

@jakebailey
Copy link

jakebailey commented May 5, 2023

Does anyone have a concrete suggestion for adding an option that would govern sorting of named imports within curly braces, whether single or multi-line? Note that with multiline, comment attachment becomes quite tricky wrt autofix.

I would recommend using the same sort order that simple-import-sort and dprint use, which in effect is to use:

const comapre = new Intl.Collator("en", {
  sensitivity: "accent",
  caseFirst: "upper",
  numeric: true,
}).compare;

Which is to say "sort case-insensitively with natural numbers, tie breaking with plain string comparison". simple-import-sort does it slightly differently via an explicit fallback, but I think those collator options should work.

We're actually looking to even this behavior out across tooling and get TS's organize import to be able to handle them evenly (via some preset mechanism).

That, and, ignore the type qualifier when sorting inside of curlies, which I'm currently trying to get finished in TS as well (right now we always put them at the end, but dprint and simple-import-sort more correctly place them inline, which prevents total reorderings when adding/removing the modifier). It's just pretty tricky to implement and I probably won't have it ready before TS 5.1 ☹️

All in all, it's easiest to just play around with https://dprint.dev/playground/#code/Q/language/typescript which should show what I'm meaning.

@ljharb
Copy link
Member

ljharb commented May 5, 2023

@jakebailey sometimes people want ascending, sometimes descending; using Intl would be a fine implementation suggestion if we could use it, but we can't because we support older nodes than where it's available. I'm asking about the schema for this option.

@jakebailey
Copy link

sometimes people want ascending, sometimes descending

Who wants descending?

using Intl would be a fine implementation suggestion if we could use it, but we can't because we support older nodes than where it's available

Intl.Collator has been present in Node since 0.12, so I would be shocked if you couldn't use it...

I'm asking about the schema for this option.

Er, wouldn't that depend on its functionality and implementation? e.g. TS only had a couple of knobs, but has gained a few more (hidden for now) in 5.0, and may gain a couple more in the future as we refine it to try and accept the various different sorting mechanisms.

@manuth
Copy link
Contributor Author

manuth commented May 5, 2023

I think the same settings like the ones mentioned in alphabetize should cover everything you'd ever want to configure:

  • order: use asc to sort in ascending order, and desc to sort in descending order (default: ignore).
  • orderImportKind: use asc to sort in ascending order various import kinds, e.g. imports prefixed with type or typeof, with same import path. Use desc to sort in descending order (default: ignore).
  • caseInsensitive: use true to ignore case, and false to consider case (default: false).

I think we could even apply the same default settings as the alphabetize settings...

Not sure about the name, though... what about alphabetizeMembers, alphabetizeNamedImports or something similar?

@ljharb
Copy link
Member

ljharb commented May 5, 2023

huh, good call, i didn't realize Intl was available in node 0.12, for some reason I thought it was only available by default in v12.

I would assume that the hope would be to design the entirety of the functionality before adding it, and I agree with @manuth that the same settings makes sense.

@jakebailey
Copy link

huh, good call, i didn't realize Intl was available in node 0.12, for some reason I thought it was only available by default in v12.

Checking MDN, the API is available, but only supported en-US until v13, which is thankfully the "right" one to use for this particular application (based on the behavior of other tools).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

6 participants