-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Alphabetize imports within groups #1105
Conversation
…n groups Fixes import-js#1406. Fixes import-js#389. Closes import-js#629. Closes import-js#1105. Closes import-js#1360. Co-Authored-By: dannysindra <als478@cornell.edu> Co-Authored-By: Radim Svoboda <radim.svoboda@socialbakers.com> Co-Authored-By: Soma Lucz <luczsoma@gmail.com> Co-Authored-By: Randall Reed, Jr <randallreedjr@gmail.com> Co-Authored-By: Jordan Harband <ljharb@gmail.com>
docs/rules/order.md
Outdated
@@ -1,7 +1,7 @@ | |||
# import/order: Enforce a convention in module import order | |||
|
|||
Enforce a convention in the order of `require()` / `import` statements. | |||
+(fixable) The `--fix` option on the [command line] automatically fixes problems reported by this rule. | |||
+(fixable) The `--fix` option on the [command line] automatically fixes problems reported by this rule. When the `alphabetize` option is used, multiple fix passes may be required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's pretty important to try to avoid multiple passes.
docs/rules/order.md
Outdated
@@ -164,6 +163,26 @@ import index from './'; | |||
import sibling from './foo'; | |||
``` | |||
|
|||
### `alphabetize`: boolean | |||
|
|||
By default, no ordering within groups is enforced. When `alphabetize: true` is used, imports must be alphabetically ordered within their groups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit restrictive; will people always want a-z? what about z-a? What about capital letters? Should it be a, A, b, c, D
, or A, a, b, c, D
, or a, b, c, A, D
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great questions. When I took a run at this in the past, I got some pushback around finding a balance between flexibility and simplicity.
Today's approach was more conservative, and the results aren't perfect. For example, we'd love to be able to differentiate webpack aliases from external node modules, and to sort asset-type resources such as .css
separately from ordinary modules.
will people always want a-z? what about z-a?
People don't seem to care much about absolute ordering at all today; I'd be a little bit surprised if many people wanted z-a
. My real fear is people wanting an arbitrary comparator. It's not difficult to add, but it's difficult to express declaratively, like in an .eslintrc.json
file.
Basically, I want a completely stable sort, and alphabetical is pretty politically-neutral.
What about capital letters?
localeCompare accepts some options that it would be straightforward to thread through declaratively from the eslint config, if that's something users want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, I want a completely stable sort, and alphabetical is pretty politically-neutral.
I, as a person who's interested in using this lint, second this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb what's your thought on @duncanbeevers response to your comment here? I am thinking a-z ordering should be good enough for majority of users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a-z and z-a is a minimum, as well as an option that chooses between “z comes before/after A” and “z is right next to Z”. I definitely don’t want an arbitrary comparator.
We ended up writing a custom eslint plugin that we could tune to support our specific ordering rules. I closed this issue since I no longer need the functionality myself. If someone wants to take this to completion feel free, but I'm setting it aside. |
@duncanbeevers Can you show your custom plugin in a gist? Recently my team has run into the shared chunk warning in the mini-css-extract-plugin documented here: webpack-contrib/mini-css-extract-plugin#246 Alphabetizing and grouping (creating a deterministic, consistent order of imports across modules) may fix the shared chunk CSS import problem. Interested in this PR, and might be interested in contributing if my company will give me time to. |
I second the motion. 😄 |
@ljharb Hi, I am interested to use the ordering here. Do you think @duncanbeevers 's PR is close to completion? What else would be necessary in order to get this PR merged? I probably can help if anyone can give me a direction |
@dannysindra there’s a few open comments; it needs a rebase. If you’d like to help, please don’t open another PR, but please do post a link to your branch here (with these rebased commits, and any new ones) and I’ll be happy to update the PR with them. Thanks! |
@ljharb I finished making the changes in my repo: https://github.com/dannysindra/eslint-plugin-import/tree/alphabetize. So I made the following configs for the
Please let me know if you have any feedback / questions. This is my first contribution btw, so I apologize in advance if I made beginner mistakes here and there |
@dannysindra i was hoping for a rebase, not a merge (no merge commits) but i've gone ahead and updated this branch with a rebased version of its, and your, commits. I'll give it a review once tests are passing. |
@ljharb thanks for the quick response! My bad, I tried to rebase and somehow must have pulled and merged. I am trying to understand what causes the tests to fail here.. it passes locally and on some env in the AppVeyor; but failed on others? Also, is there a way to run the coveralls locally? I cannot tell which lines / files decreased the coverage.. |
@dannysindra make sure you're checking out the rebased version; the travis tests are failing too. |
@dannysindra thanks, updated (next time, please delete your local branch, and re-check it out from 3db6bd7) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@dannysindra There are conflicts, so you can start by rebasing the branch. |
This still needs a rebase. |
Definitely want this! :D |
Any progress on getting this merged? |
rebased : --> https://github.com/stropho/eslint-plugin-import-1/commit/e6a5ac36c0c6024ae6f9f2629c96830d17ef586a |
Want it ! |
docs/rules/order.md
Outdated
```js | ||
import foo from 'foo'; | ||
import bar from 'bar'; | ||
import Baz from 'Baz'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These examples make it unclear whether the sorting happens by module names or by bindings names. Making them different would help (this applies to all new examples in this file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here I though the code is all good and I just need to rebase it.
Totally agree with you about this one
Any progress on this? I'm ready to contribute to this feature |
Good to hear. Right now, it's in a state of waiting for the maintainers to express themselves - and possibly merge it #1360 |
aae9da7
to
8224e51
Compare
…n groups Fixes import-js#1406. Fixes import-js#389. Closes import-js#629. Closes import-js#1105. Closes import-js#1360. Co-Authored-By: dannysindra <als478@cornell.edu> Co-Authored-By: Radim Svoboda <radim.svoboda@socialbakers.com> Co-Authored-By: Soma Lucz <luczsoma@gmail.com> Co-Authored-By: Randall Reed, Jr <randallreedjr@gmail.com> Co-Authored-By: Jordan Harband <ljharb@gmail.com>
In order to provide absolute ordering, rather than just group-wise ordering, this PR offers an
alphabetize
option that enforces locale-aware alphabetical intra-group ordering.It makes no effort to provide a more robust
fix
experience, so multiple passes may be necessary to bring a set of imports into a stable configuration, though since an absolute ordering is available, a single-passfix
probably isn't unreasonable.Fixes #1406.