Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: add option
followTsOrganizeImports
fororder
rule #287New 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
base: master
Are you sure you want to change the base?
feat: add option
followTsOrganizeImports
fororder
rule #287Changes from all commits
77e6cd3
12c17a9
31854ca
dd1f012
7b42ee0
2790318
9a0edb7
e47f0ad
f2b6011
605a9f8
708b5cc
3ea0b5b
6a1397f
213eb21
923ffb4
48fc805
3eaeedc
a80faf7
e4ff449
f5ab8e8
e9d728b
ab4ec1d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The order in
defaultGroupsTsOrganizeImports
appears to be inconsistent with TypeScript's LSP Organize Imports behavior. Based on the tests and documentation, TypeScript organizes imports with 'builtin' modules before 'external' modules, but this implementation has 'external' before 'builtin'. Consider swapping these two entries to match TypeScript's actual ordering behavior.Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
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.
@Shinigami92 Can you double check?
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 also think
builtin
should be front ofexternal
.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'll check, but my gut feeling tells me either we missed a import-statement case, or it works correctly right now already and the AI is hallucinating
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.
AH! so an
external
import is something like e.g.glob
👍Will add to test
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.
Do you want to get a test case for separated
node:*
withfollowTsOrganizeImports: true
?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.
Wouldn't the group order be different?
Thta's would be appreciated.
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.
No, why should it?
Added
ab4ec1d
(#287)You should take a case like I added here, and copy-paste it into your VSCode and then hit CTRL+SHIFT+P and run organize imports, then you can experiment yourself a bit with it 🙂
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.
Finally we got failing test cases, I don't know whether it's expected to you.
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.
Didn't the default order
external
should be listed in front ofbuiltin
as you set? Did I miss anything?I also use this command a lot when I develop projects not using this plugin nor
simple-sort-imports
, etc, for example: https://github.com/backstage/backstage.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.
todo: test case is failing 👀
I will investigate
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.
Finally we got the failing test case. 🤣
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.
Okay, it looks like the code is not prepared to allow groups separated from each other without affecting each other. Due to I spend now a lot of time into this, I think I need to first dig deeper into the code and e.g. prepare myself with the other mentioned PR to add JSDocs, because right now it is really hard to understand whats going on and you need to often switch between docs and code instead of just reading from the code.
My personal case would be fixed/satisfied by the current implementation, because I never separate imports into groups and just trust the TS LSP by doing its thing. But I came in conflict with the
'import-x/order': 'error'
rule, because by default the#
-imports conflicts with the LSP in the default case. So I come constantly in conflict in a project with a coworker right now, because he wants to use the eslint rule default and I want to use the TS LSP default..., and IMO at least the both in combination should not at least conflict by default.But yes, you are right in the sense of if now someone else would group their imports into
'node:*'
having at the top, the LSP would not trigger, but the eslint rule would now trigger. 🫠 But was it like this also before?If I overlook something right now, please hint me in the right direction what I need to touch to allow separate groups without affecting each other.
Otherwise, see you after 28th April 🚗-🇮🇸-😎
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.
It could be,
node:
is only supported in a short time at import-js#3173 which we haven't adopted into this fork yet, so testing for it is lacking right now.In my experience, TS LSP only sort
imports
in samegroup
, what means if there is no lines betweenimports
then they will be sorted, when there are new lines betweenimports
, they're considered differentgroups
by TS LSP and will not be sorted, so I thinkprivate
is not agroup
now, it should be ranked if only in samegroup
which is hereeslint-plugin-import-x/src/rules/order.ts
Lines 606 to 661 in 95dd356
Feel free to take a break, have a good time!