-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[import/order] sort order within groups #389
Comments
Can you provide an example of what you want that isn't working? |
Yeah, sorry, say for example you have the following files:
import a from '../a';
import c from '../c';
import b from './b';
import d from './d'; And eslint is configured with: import/order: [2, {groups: ['parent', 'sibling'], newlines-between: 'always'}]
sort-imports: 2 When running eslint on (e.g |
I like the idea, but I'm worried that we'd get a lot of requests to support plenty of types to order the imports. I know of a few that are requested already
I could very well see an alphabetical order too, but by the name of the variable it will be affected to (not sure how that'd work with destructuring), which makes more sense IMO as that makes looking for a variable declaration easier. Don't know if people will want other types of sort, but I think we could be surprised. We'd probably need support for exceptions too (personally, I always like to have Maybe I'm overthinking it, but this seems like potentially quite a lot of work to maintain. That said, if we're willing to commit to this work and/or accept only much requested sort options, I'm all for it. |
+1 to @jfmengels on this being a lot of work. The simplest thing I can imagine doing would be to allow a group-sort comparator function to be configured as a rule option. Such a function would take the two Then the community could support modules or shared configs with comparators that folks like. In the event that one rose to the top, we could integrate it (but the beauty would be: we don't need to). I would accept a PR implementing such a comparator option for this rule, pending @jfmengels' consideration. |
I like the module idea (even though it sounds a bit overcomplicated/tedious for what it is). This way, if we chose to integrate one or a few into the project, we could, simply by first looking for it in the "internal modules", then in external modules. I'd be fine with this too. We'll have to figure out what data to send to the comparator to at least sort in every way I mentioned in my previous comment. |
I was thinking it would be mandatory to use JS config, and the config file itself would be responsible for |
Huh, didn't even know that was possible, and that you could pass non-JSON values like functions. If we'd go and require it, would that cause problems depending on what npm version you'd use (nested module structure in npm 2 vs flat module structure in npm 3)? I think it'd be a shame to force the whole config to be a JS file just because of this option (if option is used) if we can avoid it, though it would be a simpler solution. We could start implementing first using a JS only solution using a function as param, and when we're happy with how it's working, accept a string and require a module. |
I think of it sortof opposite: it's fantastic that the option is there to use pure JS config, for esoteric options like this. I'm making an unfounded assumption that this is not an option that will see frequent use. That said, since some strategy will potentially find dominance, the |
Agreed, but since you can't merge a js config and a YAML/JSON config, you'd have to port your entire config file from YAML/JSON to JS just to activate this option.
Hmm... I would assume the opposite (if the option is easy to use). People who use this rule already want to sort their imports, this is just one natural step further. I would personally activate it (probably the staircase rule). Just thought of the exceptions argument I made. You'd probably want to allow settings in your comparator too, probably something like this: {
"import/order": ["error", {
"groups": [
"builtin",
["sibling", "parent"],
"index",
],
"newlines-between": true,
"sort": [
{
"comparator": "sorter-module-1", // string or function
"exceptions": ["lodash"] // or however you give options
},
{
"comparator": "sorter-module-2",
"reverse": true
}
]
}]
} Man... the settings for this rule could use it's own file at this rate... Like I said earlier: this is a rabbit hole option (and the rule even more so). |
Yep, it could get wild. You might almost say you could build a whole plugin for it... 😉 |
Upon reflection, I think you're right--people will probably love and use this. I am just projecting too much. I'm happy to support however you want to come at this. |
I think a simple alphabetical inner sort would cover a lot of use cases and likely be very simple to implement (vs a super generic / powerful mechanism) |
Would this sort, and thus, extend options for ordering -import Foo from 'foo'
-require('bar')
-import Baz from 'baz'
require('fizz')
\r
+import Baz from 'baz'
+import Foo from 'foo'
+require('bar')
require('fizz')
\r |
@kevinSuttle If that's possible, that'd be awesome, but I'm not sure that's it's easy to do (if requires are all over the file). Anyway, let's do this. I have other things I'd like to work on currently, so I won't work on this for the foreseeable future. |
@kevinSuttle AFAIK it's not valid ES to have imperative statements before (imports are processed before any code in the module is executed) |
I thought it was valid, but that it was hoisted (like functions). |
Oh, I don't mean putting anything before imports. I'm asking about this rule treating |
@nevir: @jfmengels is right, they'll be hoisted, but it's still valid to have statements before them. Just might result in confusion. (thus, the |
aha! k; thanks for fixing my understanding on that 👍 |
* After sorting imports by group, allow sorting alphabetically within a group * import/order rule now accepts sort-order option that may be 'ignore' (default) or 'alphabetical' * Fixes import-js#389
* After sorting imports by group, allow sorting alphabetically within a group * import/order rule now accepts sort-order option that may be 'ignore' (default) or 'alphabetical' * Fixes import-js#389
* After sorting imports by group, allow sorting alphabetically within a group * import/order rule now accepts sort-order option that may be 'ignore' (default) or 'alphabetical' * Fixes import-js#389
* After sorting imports by group, allow sorting alphabetically within a group * import/order rule now accepts sort-order option that may be 'ignore' (default) or 'alphabetical' * Fixes import-js#389
* After sorting imports by group, allow sorting alphabetically within a group * import/order rule now accepts sort-order option that may be 'ignore' (default) or 'alphabetical' * Fixes import-js#389
* After sorting imports by group, allow sorting alphabetically within a group * import/order rule now accepts sort-order option that may be 'ignore' (default) or 'alphabetical' * Fixes import-js#389
Ah, I found #629 - that has changes requested on it, and the contributor who requested them is unavailable for personal reasons. The thing to do is "be patient" - I know it's difficult. |
Oh I'm not saying this project is not maintained, just that forking it might be a way to move this forward. Which would require someone who wants to maintain the fork. If there's anyone who wants to do that we could use the fork and maybe merge back in the future if the project owner wants to. |
You can npm install from the git branch for the PR, if you want to test it out. |
I know. |
As far as I can tell, the EDIT: But still a bit quirkily, see #389 (comment) and #389 (comment) I now recommend eslint-plugin-simple-import-sort instead. |
concerned parties: please see #951 and comment there if you have ideas on how to manage/govern the development of the |
@lydell Unfortunately the autofix of https://github.com/marudor/eslint-plugin-sort-imports-es6-autofix and the If you manually resolve these violations the result will pass this config, but that's a pain I'm interested in avoiding. Unfortunately I'm not aware of a way to run these rules separately in a specific order other than running eslint twice passing a different config each time, which doesn't really work in the case that you want to opt-in specific directories of your application gradually. |
@gdaolewe Yeah, I've also noticed that problem after using both at the same time for a little while. It works OK-ish, but there are definitely (solvable) problems every now and then. |
What is the status of this enhancement? Has it been merged? will it be merged? |
any update...? |
Any update? Issue has been open for 3.5 years. I could have hand crafted a rare faberge egg in that time. |
Two, even, and if you had, and sold them, and donated the funds to sponsor the project's maintainers, then it would have been much easier to set aside the time to fix it :-D #629 and #1105 and #1360 all attempt to address this. These are on my queue to look at, and I will do so as soon as I can find the time. |
…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>
is it possible to sort based on variable name instead of import path within a group? |
Variable name is just a local choice; the path is what’s meaningful. |
let's see from user's perspective.. you open a file with 500 lines and 15 imports. You are looking to see if this file uses some custom module, so path isn't gonna be like TLDR: Variable names are easier to read because they are not nested and highlighted by IDE themes by default. So sorting that has compounding advantage. It's making better option more better. I'm thankful for all the work and it's 100x better than nothing. But I'd like to suggest a way to sort based on variable names instead of path name. |
If you have that many imports, wouldn't you want to use search anyways, since that'd always be fastest? |
It'd be handy if
import/order
either played nice with eslint'ssort-imports
rule (if that's feasible?) - or implemented something along the same lines, but on a per-group basisThe text was updated successfully, but these errors were encountered: