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

Support for namespace imports #144

Closed
szku01 opened this issue Sep 11, 2023 · 8 comments · Fixed by #149
Closed

Support for namespace imports #144

szku01 opened this issue Sep 11, 2023 · 8 comments · Fixed by #149

Comments

@szku01
Copy link

szku01 commented Sep 11, 2023

Nested type declarations can be "deeply imported" in typescript, in a bit awkward syntax, see:
microsoft/TypeScript#39935

Currently both the plugin and the builtin import order rule ignores these lines, which is rather unfortunate, since editors insert autoimports below the last "import-like" line:

import { Store } from '@ngrx/store';
import { BehaviorSubject, combineLatest } from 'rxjs';

import { ViewModels } from 'src/generated/api-services-generated-new';
import { ApiCallStatus } from 'src/tools-new/api-action-statuses.reducer';
import { IToolsState } from 'src/tools-new/tools.reducer';

import Search = Models.Dashboard.Search;
import IUserStats = Models.Dashboard.IUserStats; // <--- this is not the same as a type redeclaration

An autoimport will insert the new import { foo } from './foo' below the last line, which will be ignored by the sorter. Any new imports will land under the last import, further piling up the problematic (unsortable) lines.

I can understand, if this can / should be considered a problem with the code editor (vscode in my case) though. What's your opinion?

@lydell
Copy link
Owner

lydell commented Sep 11, 2023

Hi! Do you mean that you would like lines like these:

import Search = Models.Dashboard.Search;
import IUserStats = Models.Dashboard.IUserStats;

to be sorted, together with other imports? If so, what should we sort on – the thing after the =? How should they interact with other imports?

Just as a heads up, I have never seen such imports before, so I’m not very excited about putting work into them.

@szku01
Copy link
Author

szku01 commented Sep 12, 2023

@lydell it's not very popular, but type/interface (re)assignment is not the same as deep interface references and currently this is the only way to do it (the linked github post agrees with both you and me, that it is confusing) - but I started working with a huge codebase and this is all over the place, because we have lots of nested, complicated interfaces (at this point I'd say that "if you need it, then PRs are welcome", but there's a very very low chance I'd be able to squeeze something like this into the scrum board).

Imo these lines can be left alone, the problem is that the sorter ignores anything that comes after these, and vscode inserts the autoimports after these lines (hence I said it is debatable who should do this). Example:

(1) we have this set of imports:

import { bar } from './bar-utils';
import foo from 'foolib';

import ICorgi = IAnimal.ICanine.IDog;

(2) we start coding and let vscode add an autoimport (qux):

import { bar } from './bar-utils';
import foo from 'foolib';

import ICorgi = IAnimal.ICanine.IDog;
import { qux } from './qux-utils'; // <-- newly eddid by editor

(3) use autofix (current):

import foo from 'foolib';
import { bar } from './bar-utils'; // <-- these are properly sorted

import ICorgi = IAnimal.ICanine.IDog;
import { qux } from './qux-utils'; // <-- this was not touched

expected:

import foo from 'foolib'; // <-- external lib first
import { bar } from './bar-utils'; // <-- internal code follows
import { qux } from './qux-utils';

import ICorgi = IAnimal.ICanine.IDog; // <-- remains untouched

@lydell
Copy link
Owner

lydell commented Sep 12, 2023

FYI the reason this happens is because this plugin sorts “chunks” of imports – import statements in a row. In this case there are two chunks, because the namespace import thingy happens to be a different AST node.

I’ll need to think about what to do here.

@lydell
Copy link
Owner

lydell commented Sep 16, 2023

Here are some things I’ve learned:

  • I think TypeScript calls this “import assignment”.
  • import identifier = can be followed either by a namespace TSQualifiedName, or require("...").
  • There’s also export import identifier = thing.

So the same feature supports both the namespace thing mentioned in this issue, and import x = require("..."). This plugin does not support require, so I won’t support import x = require("...") either.

We could support just import x = A.B.C. I don’t really like only partially supporting a syntax, but let’s keep going for a bit. Then, what does it mean to support it? I’d say that they’d be possible to sort among other imports with the groups option. But these imports are completely different, since they don’t have from to sort on.

Another way would be to simply “move them out of the way” (and potentially alphabetizing them). But should they be moved above or below? This also feels a bit random – if you have other statements that aren’t standard imports in between imports, those aren’t moved away (I recommend import/first there). I guess the difference is “because they start with the word import”.

I could maybe see a future where this plugin moves import a = A.B.C in chunks of imports to the bottom and sorts them alphabetically (not customizable in any way), and ignores import a = require("a"). I have not thought about the export import thing yet.

But I’m currently leaning more towards not making any changes. Instead, you could create your own rule that moves import assignments like you want them.

@szku01
Copy link
Author

szku01 commented Sep 18, 2023

Very good points, haven't know about the export import require horror :) Honestly, I don't much care about the import assignment block (or its order), the only problem now is that "real" imports land after the import assignment group. If you could move those "back" into the other real imports, then that would be it.

The other option is that I create a vscode plugin for myself, where I chain format document + sort imports + eslint fix (sort imports moves the bottommost import to the "real" group, eslint fix runs - among other things - your plugin).

@MillerSvt
Copy link
Contributor

I fixed this problem without adding a lot of code and complex logic. This problem is very urgent for us, we have broken imports for this reason. Other rules support sorting of this type of imports, so it would be nice to add it here as well.

lydell pushed a commit that referenced this issue Feb 8, 2024
@lydell
Copy link
Owner

lydell commented Feb 8, 2024

Released in v11.0.0. (Finally! Sorry for the long delay.)

@lydell
Copy link
Owner

lydell commented Feb 10, 2024

Reverted in v12.0.0.

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

Successfully merging a pull request may close this issue.

3 participants