Option to allow first-specifier/alias import ordering

[tdd]

Introduces an option (disabled by default) to switch from lexicographical module path ordering to natural-language first-specifier/alias ordering when organizing imports.

• Closes #316

Description

It's fairly common to order imports based on the first imported identifier (specifier, alias or default alias). TSH currently only sorts by module path, and lexicographically, too (which isn't necessarily the dominant mindset).

This adds a typescriptHero.resolver.organizeSortsByFirstSpecifier boolean option, defaulting to false, that enables just such a behavior. Import grouping is unaffected: sorting happens within groups.

Sorting is locale-aware, by the way, so not just lexicographical. When looking for specifier-ordering, we usually are in a "human language" mindset, so case shifts and diacritics should work in natural order.

Documentation, config schemas and tests are updated, linting is green.

[tdd]

Hey @buehler! I went and sent #335 and this one, I strived to meet all requirements you may have, hope you like it! These would really close the deal for my company and trainees on TSH 😉

Looking forward to seeing what you think,

Best,

[buehler]

Your code is awesome! :-)
One small thing to change, the rest is pretty perfect.

[buehler]

Please add this function as a non exported function (with jsdocs) in the root of the file.

This function would be defined each time you call it, since it does return the correct specifier and is not bound to a closure, you can specify it in the root (top or bottom does not matter.. I'd use the top, beneath the imports)

[tdd]

Dang, that's obvious, should have caught on that. Hang on a sec.

[tdd]

There you go, @buehler. The moment you merge this in, #335 will become un-mergeable due to both-added conflicts in the file you want me to re-indent anyway, I'll address this then.

[buehler]

@tdd Whopsie. I'm sorry, I merged the wrong one first. I'll review the changes..

edit: would you mind to resolve the conflicts? Thanks for your work on this extension!

[tdd]

Aaaand… there we are, @buehler! Nicely rebased and working. Looking forward to seeing that merged in.

Btw, I notice you manually squash-merge (of sorts, preserving original committer info) and commit results directly to master, instead of the usual merge-based PRs (except for your stuff, apparently?). Just curious: are you doing this using GitHub’s recent UI changes (PR merge choices), or locally with Git?

Best,

[buehler]

Hey @tdd
Thanks! Gonna merge right away.

Actually, I use the github UI. The workflow I use is the following:
feature / fix / other stuff branches are squashed and merged into develop. With that, I can ensure a certain commit message structure which is then recognized by semantic release and correctly parsed into a changelog and release notes.

That's why I squash&merge all pull requests into develop and when I want to release a version, it's "merge" only into master (from develop).

The contributors are still accounted for, github does some magic with that.

There was one time where I directly merged something into the master, but that was a mistake made by me ;-)

[tdd]

Hey @buehler, just a quick question about this recent set of upgrades…

I'm training professionals on React and Node on a monthly basis, and our recommended editing stack is based on VSCode and a set of extensions, including yours. I'm looking forward to having my recent contributions show up in the installed ext, any idea when you're planning to push a new release that includes these?

FWIW, my next training session starts next Tuesday, and we'd enjoy this specific set of features starting on next Wednesday. Then I'm teaching Node the next week.

If you're waiting for some other stuff to come in before pushing 1.8, that's fine. I'm just curious about when I might expect that to come in.

Best,

[buehler]

Hey @tdd
No worries! I plan to release this awesome features of yours this week. There is a big "no no" right now going on with #331. And I did "fix" the recognizer part in #339. But there is an open question since the findFiles method does produce different results on different operating systems (microsoft/vscode#37806).

But despite that fact: A big thanks for the flowers 😊
I'm thrilled to be a part of the help you guys need to work!

Actually the last thing that definitely needs to be fixed is that crux with #331. After that, I'll release 1.8.

Regards
Chris

[tdd]

Hey Christoph (I'm Christophe myself; the German-style spelling like yours always feels like it's missing something to me 😉 I guess English-speaking people feel the same way with my FR spelling 😁 )

About #331: I experienced a shitload of EPIPE's on my usual projects myself, not due to using 1.7.0, but the moment I upgraded to Code 1.18. Go figure. Perhaps this is because 1.18 auto-imports like a boss (it also works on pure-ES node_modules contents, unlike TSH), thereby stepping over TSH's toes in some weird way?

Anyway, best of luck with figuring this out! Sounds super-hairy and, more importantly, borderline impossible to reproduce in a reasonable way 😞

[tdd]

(for some reason I'm not getting all the message notifications from GitHub; have to keep thinking about coming back and checking the threads out…)

[buehler]

Hey Christophe :-)
Well it seems as TSH has reached the end of its usefulness. Maybe I'm just reducing the functionality to organize the imports and show the document outline.

We'll see, as soon as I fixed the messed up error :-/

[tdd]

Hey @buehler !

So what's going on with 1.8.0? You seem to have fixed #331 a while ago, but no new version is out?

Also, Code's auto-import feature is still super broken on non-TS code, using absolute paths or weird relative paths instead of resolved ones in many cases. So TSH is still very much useful! But mostly I'm fond of the import organization features (Code has none), and I can't wait for the new sorting options and proper group ordering to be out!

Let me know!

Best,

[buehler]

Hey @tdd
I'm still experiencing the errors with travis :-(
Currently I disabled them. I hope I can use them again asap.

Right now, I'll release the 1.8 version with another important fix and after that I'm going to disable the import adding feature of TSH. I think the future of TSH will be organizing and refactoring.

Have a nice day!
Regards

[tdd]

Hey @buehler

I'm delighted to see it in my VS Code, however I'm baffled as the new "order by first specifier" option doesn't seem to work at all? It worked great during dev and in tests…

Made sure workspace settings were used, tried restarting Code, etc. No dice. It sorts by module name no matter what.

o_O

[buehler]

Hmmm. That's kinda strange.

If I got some time, maybe I'll find out something.
(Nearly as strange as the failing tests only on travis)

[tdd]

Hey @buehler !

OK I checked, and found out what's wrong. The thing is, this requires some refactoring and I need your input.

The gist of it is, many of your tests assume that sorting imports within a group is the job of the generate() call. You even have tests for this. These are made, among other things, in ImportManager#calculateTextEdits(), which are ultimately responsible for the actual code organization in the editor from within ImportManager#commit().

The problem is, once we introduce an alternate sort system, as we do AOT in ImportManager#organizeImports(), we get overwritten. BTW, this method already did some sorting, only to have it done again in generate(). This smells of organic codebase evolution with a duplication of responsibility that went unnoticed until now.

The tricky part is, import groups have no access to / knowledge of the sorting config option, so they can't pick the proper sorter function on their own. So we have two paths here:

• Decide that it's not generate()'s job to sort, except perhaps to reverse sorting if order is desc, but that's it. So its sortedImports getter should only reverse if needed, assuming ascending order is already there. This needs investigation as to what other code paths rely on generate() and whether some of these assume builtin sorting. Tests expect this, but is that a functional approach or just a unit approach, I'm not sure.
• Conversely, decide that yes, only generate() should sort, which means we don't need to do it at all in organizeImports(), whose sole responsibility should then be (a) prioritize regex groups to ensure proper capture despite keyword groups and (b) triage into groups. This means we need to inject groups with the current sorting option value (lib-name / first-specifier), so they can ultimately sort accordingly.

I'm willing to implement either of these on top of master/v1.8.0, but I need your input on this, obviously.

Best,

[buehler]

Hey @tdd

Honestly, I like the second option better. It's the cleaner solution to decuple those processes and leave the sorting part to the entities that hold the imports. I'm feeling lucky that I added DI in the past ;-)

So that would mean we need to change the behaviour and use the organize funtion to create the groups and the groups themselves are responsible for sorting.

I havn't had much time to code on this project in the past few weeks, but my guess is, over the holidays I'm gonna do some coding ;-) I'd be happy to help you in any way needed and I'll probably show TSH so love too and fix some other problems 😃

Regards and some nice remaining days before the holidays..
Chris

[tdd]

Hey @buehler,

Second option is fine by me, however I have no idea how to provide the document-relevant ext config in a DI/IoC way using TS and the architecture you have in place. I don't want to blindly copy-and-paste from ImportManager, so do you think you could just take a moment to push a change to the import group classes so they, too, get a this.config equivalent to ImportManager's?

What troubles me most is that this needs a document URI, but import groups have no document to work on…

Alternatively, they could just get the config flags from the import manager using their constructors, but I'm guessing that's not how you want it?

[tdd]

(btw, this brings common impl code to all import groups; can't TS interfaces have actual code implemented in them? Or would we need to turn it into an abstract class?)

[buehler]

@tdd I'm still owing you some code.
Nope, interfaces can't have code, but actually we could make some base classes for that.

Hope you had a nice new year party ;-)

[buehler]

Alright, I made you a branch:
https://github.com/buehler/typescript-hero/tree/refactor/import-groups

2ecee9e#diff-a8e672232745d365e134171621961a96R21

Like that, you can inject stuff that is not directly constructed by the IoC Container.

Btw: do you know any good resources on how to handle recursions (i.e. flatten them into iterations)?

Regards