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

Case-insensitive sort of named imports #7

Closed
kachkaev opened this issue Feb 24, 2019 · 18 comments
Closed

Case-insensitive sort of named imports #7

kachkaev opened this issue Feb 24, 2019 · 18 comments
Labels

Comments

@kachkaev
Copy link

kachkaev commented Feb 24, 2019

👋 @lydell!

After switching from TSLint to ESLint and your plugin, I've noticed that named imports are sorted differently. Your approach is case-sensitive, while TSLint's ordered-imports is not by default.

// tslint
import { a, B, c } from "somethiing";

// eslint-plugin-simple-import-sort
import { B, a, c } from "somethiing";

The former notation feels more natural to me. WDYT?

@kachkaev kachkaev changed the title Case-insensitive sort Case-insensitive sort of named imports Feb 24, 2019
@lydell
Copy link
Owner

lydell commented Feb 25, 2019

I actually prefer the uppercase ones (CONSTANTS, Classes, Types) coming first.

@aMarCruz
Copy link

I prefer functions, Classes, CONSTANTS, Types :)

@zarsky-broad
Copy link

Could there be an option to toggle this? I could write a PR.

@lydell
Copy link
Owner

lydell commented Apr 5, 2019

In my opinion one of the benefits of this package is that it has no options. I'd like to keep it like that until an option is absolutely necessary. Convince me and we'll change this!

@zarsky-broad
Copy link

That's fair! This would be an entirely optional thing, with the default being to continue working as things do now, so for the vast majority of users that are happy with the current setup, they wouldn't even need to know that an option exists. For users like me, that one boolean would mean the difference between having no option but to enforce rules manually and having an easy way to standardize it.

So I guess my argument is that for most people, it would be as if there were no options, and only people who tried it and ran into that frustration would be likely to even discover it.

@lydell lydell added the maybe label Apr 24, 2019
@Philipp91
Copy link

+1 for such an option. WebStorm/PhpStorm has an opposing opinion here and sorts case-insensitive.

I actually prefer the uppercase ones (CONSTANTS, Classes, Types) coming first.

That's fine. I personally don't care how my imports are sorted, it's not a part of the code that I really read. As long as all my tools handle them automatically, quickly and consistently, any ordering is fine.

In my opinion one of the benefits of this package is that it has no options.

Fair point. And WebStorm/PhpStorm has an option to turn off the sorting completely. However, it's just extremely convenient to be able to hit Ctrl+Alt+O in the IDE and having the imports sorted in an instant. Perhaps there's a way to put the simple-import-sort autofix on a hotkey, too, and I'd be very interested to see how that can be done. Anyway, the builtin sort would still be much faster.

Whether or not WebStorm/PhpStorm can be configured differently, the fact that a major IDE's default behavior opposes the only behavior of this plugin warrants adding an option, IMHO. It doesn't make it "absolutely necessary", but it might convince a few users not to drop the plugin.

@Livven
Copy link

Livven commented Jun 9, 2019

I don't care about which way to use, but agree that fewer options are better. Since in this case most established tooling seems to be case-insensitive (examples being TSLint and WebStorm) I think that would be the obvious choice here as well.

Whatever your decision though, thanks for the great work :)

@lydell
Copy link
Owner

lydell commented Jun 9, 2019

Following TSLint's and WebStorm's lead sounds like a good idea! Three is the magic number, though. Can you find one more popular precedent?

Also, how do TSLint and WebStorm work when it comes to sorting the import statements (sorting on from)? Is case insensitivity used there too? Should it?

@zarsky-broad
Copy link

IntelliJ/WebStorm sorts case insensitive on from:

import { buttonPrimary, linkButton, Select, spinnerOverlay } from 'src/components/common'
import DataTable from 'src/components/DataTable'
import ExportDataModal from 'src/components/ExportDataModal'
import FloatingActionButton from 'src/components/FloatingActionButton'
import { icon, spinner } from 'src/components/icons'
import { IGVBrowser } from 'src/components/IGVBrowser'
import { IGVFileSelector } from 'src/components/IGVFileSelector'
import { DelayedSearchInput, TextInput } from 'src/components/input'
import Modal from 'src/components/Modal'
import { notify } from 'src/components/Notifications'
import { FlexTable, HeaderCell, SimpleTable, TextCell } from 'src/components/table

@lydell
Copy link
Owner

lydell commented Jun 15, 2019

Thanks!

Status: I’m now researching using String.prototype.localeCompare/Intl.Collator for a more human sort.

const collator = new Intl.Collator("en", {
  sensitivity: "base",
  numeric: true
});

function compare(a, b) {
  return collator.compare(a, b) || (a < b ? -1 : a > b ? 1 : 0);
}

lydell added a commit that referenced this issue Jun 15, 2019
Fixes #7. Imports are now sorted case insensitively, just like TSLint
and many popular editors/IDEs. As a bonus, numbers in imports are now
sorted by their numeric value for even more humane sorting.

Fixed: `import {} from "a"` is no longer considered a side-effect
import. Only imports completely lacking the `{...} from` part are.

Fixed: ".foo" is no longer considered a relative import.

Improved: The special casing of "." and ".." is now more consistent.
Those two cases are now treated just like "./" and "../", and no other
patterns are adjusted.

Improved: Trailing spaces after imports are now preserved for better UX.
Before, accidentally adding some trailing spaces would result in a
"Please run autofix to sort these imports!" error, but the autofix
wouldn’t actually sort anything but only remove some spaces, which was a
bit weird.
@lydell
Copy link
Owner

lydell commented Jun 15, 2019

Could you all try out v4.0.0-beta.1?

@Philipp91
Copy link

tl;dr: It works as intended, all the caveats below are unrelated to sorting and harmless.

I tested the new beta with PhpStorm, where I turned "Sort imported members" and "Sort imports by modules" on.

For the first test, I essentially want PhpStorm not to destroy the eslint sorting:

  • Run eslint --fix.
  • Stage all the diffs with Git. (Most files have one diff, some have more.)
  • Run "Organize Imports" on the entire source tree in PhpStorm.
  • Look at the remaining diffs (not staged in Git).

All of the remaining diffs are formatting changes (for the better). For instance:

import {
  A,
  B,
  C,
} from 'x';

becomes

import {A, B, C} from ' x';

if it fits my configured line length.

I understand that the simple-import-sort plugin deliberately doesn't do formatting. So this is all fine -- there are no sort-related diffs left.

For the second test, I essentially ran the inverse scenario. Essentially I want to use PhpStorm for formatting, and then simple-import-sort should not complain.

  • Starting from scratch, running eslint on master results in 103 import sort warnings.
  • Run PhpStorm's "Organize Imports" on all files.
  • Run eslint again => 24 warnings from import sort, plus 11 about misplaced commas.
  • Run eslint --fix to see where the additional diffs come from.

All of the remaining (non-comma) diffs are only about where blank lines are inserted -- PhpStorm does not handle that.

In conclusion, the sort order is now 100% aligned on my project. PhpStorm's "organizing" of imports not only sorts but also ensures the line length fits. And simple-import-sort additionally inserts blank lines between blocks. Most importantly, there is no situation where I have to go back and forth between the two, i.e. they never disagree anymore. Thank you!

@lydell
Copy link
Owner

lydell commented Jun 18, 2019

I’ve tried the beta on a couple of projects myself now, and the diff churn isn’t too bad actually. And I do agree that case insensitive sorting does feel more natural now that I’ve tried it a bit for real.

@kachkaev @zarsky-broad What do you think?

@kachkaev
Copy link
Author

Hi @lydell 👋

I just tried the new version in a rather big monorepo at work and the upgrade went quite smoothly. The diffs look adequate and the resulting order is oftentimes indeed a bit easier to read. Glad that you like case-insensitive sorting too!

@zarsky-broad
Copy link

zarsky-broad commented Jun 18, 2019

I see minor thrash in the following situations, which I think are beyond the scope of this plugin to address, and which I think are acceptable (I have a macro that causes intellij to invoke the eslint fix action on a file after I run reformat on it, so any thrash is resolved immediately anyway):

  • intellij doesn't make the distinction between side-effect imports and non-side-effect imports
  • intellij doesn't know how to add linebreaks between import groups

Perhaps interestingly, we use absolute imports for internal paths, rather than relative (i.e. import * as Style from 'src/libs/style' vs import * as Style from '../../libs/style'), and as one might expect this causes them to be grouped with node module imports. It occurs to me that some people might like it if there was some way to tell the plugin to sort imports where the path matches a regex into their own group, as tslint's ordered imports allows. I'm not gonna fight for it or anything, and it certainly won't stop me from using this as-is, it just occurs to me.

tl;dr 👍👍👍 :shipit:

EDIT: the first point above caused issues with our implementation of Prism, but they have a babel plugin that they recommend using instead of the way we did it.

@lydell
Copy link
Owner

lydell commented Jun 19, 2019

Released in v4.0.0. Thank you everyone for the great discussion!

@JounQin
Copy link

JounQin commented Oct 9, 2021

@lydell

tslint has an option "named-imports-order": "lowercase-last" which is what I'm using.

I actually prefer the uppercase ones (CONSTANTS, Classes, Types) coming first.

I like the original defaults. 😅

@lydell
Copy link
Owner

lydell commented Oct 9, 2021

@JounQin Hi! This plugin was never supposed to be a replacement for tslint. I created this before I used TypeScript. I’ve only used tslint in one project that someone else created. So “tslint supports that” arguments don’t bite on me 😄

I’m not interested in changing the sorting. My advice would be to:

  1. Check if you can achieve what you want using the Custom grouping option: https://github.com/lydell/eslint-plugin-simple-import-sort#custom-grouping. Grouping can be used as sorting to some extent.
  2. Fork the plugin.

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

No branches or pull requests

7 participants