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

New Rule: ES6 sort-imports #435

Closed
benjohnson opened this issue Feb 23, 2016 · 12 comments

Comments

@benjohnson
Copy link

commented Feb 23, 2016

Hi, I'm wondering if Standard feels like sorting ES6 imports are a good standard 😸

Docs are here: http://eslint.org/docs/rules/sort-imports. This is one of those nice rules that doesn't affect ES5 code and so can be enabled without relying on new infrastructure. The downside is that this could potentially cause a lot of new linter errors in big ES6 codebases.

If people are interested in this, I can put a PR up in the sharable config.

Thanks for considering.

@feross

This comment has been minimized.

Copy link
Member

commented Feb 23, 2016

I looked at this rule and considered enabling it for standard 6, because I personally sort my commonJS require() lines. It's easy with an editor shortcut, and it helps me catch duplicate require() calls. But ESLint doesn't have a rule for sorting require() calls.

But the ES6 import rule seems super unintuitive to me, since it's enforcing sorting based on the type of ES6 import, before sorting alphabetically. And ES6 has 4 different ways to import. And no order seems particularly intuitive to me.

@feross feross added the enhancement label Feb 23, 2016

@feross

This comment has been minimized.

Copy link
Member

commented Feb 23, 2016

Sidenote: It's irritating how complicated all these new language features are. Standards folks don't seem to appreciate simplicity as much as they ought to. Arrow functions have 5 different syntaxes for example... :(

@benjohnson

This comment has been minimized.

Copy link
Author

commented Feb 23, 2016

Yep, I hear you about the complexity. One thing sort-imports does out of the box too is consider case sensitivity, so they want you to sort classes above other stuff, which seems insane to me. It results in:

import Zebra from 'animals'
import baboon from 'animals'

It's configurable, but still.

To some extent, the sheer complexity of a lot of ES6 makes linters like Standard even more important now. This one is a good example. We've made the language so complex that even though the general principle of a sorting order for imports is intuitive (general to specific, alphabetical) it's really hard to enforce without external tools.

@feross

This comment has been minimized.

Copy link
Member

commented Feb 23, 2016

Can you check how many packages in the test suite fail when you enable your desired configuration of this rule? Still not sure if the rule is a good idea, but the numbers would be interesting to see.

@benjohnson

This comment has been minimized.

Copy link
Author

commented Feb 23, 2016

I get the same results if I run it where it cares about alphabetization vs not:

# tests 420
# pass  416
# fail  4

Packages that failed:

tmp/magic-virtual-element/index.js:3:1: Imports should be sorted alphabetically. (sort-imports)
tmp/magic-virtual-element/test/index.js:3:1: Imports should be sorted alphabetically. (sort-imports)
not ok 114 magic-virtual-element (https://github.com/dekujs/magic-virtual-element)
---
tmp/cracks/lib/cli.js:3:1: Imports should be sorted alphabetically. (sort-imports)
tmp/cracks/lib/index.js:2:1: Expected 'multiple' syntax before 'single' syntax. (sort-imports)
not ok 211 cracks (https://github.com/semantic-release/cracks)
---
tmp/hjs-webpack/examples/assets-and-index-html/src/app.js:5:1: Imports should be sorted alphabetically. (sort-imports)
tmp/hjs-webpack/examples/just-assets-no-html/src/app.js:5:1: Imports should be sorted alphabetically. (sort-imports)
tmp/hjs-webpack/examples/prerendered-html-files/src/app.js:5:1: Imports should be sorted alphabetically. (sort-imports)
not ok 315 hjs-webpack (https://github.com/henrikjoreteg/hjs-webpack)
---
tmp/tape-promise/tests/async.test.js:2:1: Imports should be sorted alphabetically. (sort-imports)
tmp/tape-promise/tests/mixin.test.js:2:1: Imports should be sorted alphabetically. (sort-imports)
tmp/tape-promise/tests/promise.test.js:2:1: Imports should be sorted alphabetically. (sort-imports)
tmp/tape-promise/tests/regular.test.js:2:1: Imports should be sorted alphabetically. (sort-imports)
not ok 402 tape-promise (https://github.com/jprichardson/tape-promise)
@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2016

This would conflict with how I write code - I've found my brain doesn't automatically sort things alphabetically, but can easily recall if a word was long or not.

// alphabetically
import Zebra from 'animals'
import ape from 'animals'
import baboon from 'animals'
import cockatoo from 'animals'
import hyena from 'animals'
// length based
import cockatoo from 'animals'
import baboon from 'animals'
import hyena from 'animals'
import Zebra from 'animals'
import ape from 'animals'

I'd rather this be left up to the discretion of the author, as I feel this wouldn't have any noticable benefits.

@dcousens

This comment has been minimized.

Copy link
Member

commented Feb 24, 2016

I personally sort A-Za-z (:'<,'>sort), not Aa-Zz (:'<,'>!sort)

@benjohnson

This comment has been minimized.

Copy link
Author

commented Feb 24, 2016

@dcousens As much as it weirds me out I'm totally all for taking the eslint default if it doesn't bother other people. This is why Standard is valuable 😀

@ianstormtaylor

This comment has been minimized.

Copy link

commented Jun 6, 2016

I tend to use alphabetical because it's easy as an editor shortcut, but if the eslint defaults were enforced with the formatter on save, then this would be pretty much the same. Even though the order is slightly different than alphabetical (symbols wise), anything that's got a defined order seems fine. It's something you'll easily adjust to within a week or two of use. So +1!

@cesarandreu

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2016

I try alpha-sorting based on the import location, but sometimes you sadly have circular dependencies which affect how you import stuff. :(

Also, sometimes you have multiple long named imports:

import {
  AnotherEvenLongerImportFromFooBar,
  createSomeFoobarThing,
  SomeReallyReallyLongImportFromFooBar
} from './foobar'
@timoxley

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2016

For me, alphabetical or length-based sorting is a waste of potential information. I try to encode some kind of rough meaning to the order of imports, based on their importance or how common they are over the project, e.g. in order:

  1. shims & polyfills
  2. core deps e.g. fs or path
  3. important 'architectural' dependencies e.g. react or tape
  4. less important dependencies e.g. mkdirp or through2
  5. project-local dependencies e.g. ./things

This ordering means that for similar files in a project, the first few lines will remain roughly the same.

Regardless, I think this proposal is a non-starter given that require/import order can at times be significant e.g. polyfills MUST come first, though perhaps requiring es-lint disable comments would be a good way of flagging those dependencies which are order-significant.

@feross

This comment has been minimized.

Copy link
Member

commented Jul 13, 2016

Closing for the reasons that @timoxley and @cesarandreu stated.

@feross feross closed this Jul 13, 2016

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
7 participants
You can’t perform that action at this time.