Sort exports when organizeImports is run

[amcasey]

Note that there's no attempt to remove unused exports. I also didn't move the existing import tests into a corresponding describe for consistency because I wanted to avoid blowing up the diff.

Fixes #23640

[amcasey]

Just for you, @DanielRosenwasser.

[ghost]

Seems like the test above this one is unnecessary?

[amcasey]

Sure, deleted.

[ghost]

Since this isn't really a filter, this and parseImports could use a helper function:

function castEvery<T, U extends T>(inputs: ReadonlyArray<T>, pred: (t: T) => t is U): ReadonlyArray<U> {
    for (const input of inputs)
        Debug.assert(pred(input));
    return inputs as ReadonlyArray<U>;
}

[amcasey]

I'm not sure I understand what benefit this provides.

[ghost]

To keep it type-safe this could be written as:

function organizeImportsWorker<T extends ImportDeclaration | ExportDeclaration>(oldImportDecls: ReadonlyArray<T>, coalesce: (t: ReadonlyArray<T>) => ReadonlyArray<T>)

And called with:

const coalesceAndOrganizeImports = (importGroup: ReadonlyArray<ImportDeclaration>) => coalesceImports(removeUnusedImports(importGroup, sourceFile, program));
...
organizeImportsWorker(topLevelImportDecls, coalesceAndOrganizeImports);
...
organizeImportsWorker(topLevelExportDecls, coalesceExports);

Then inside the function you can avoid the casts and just:

const newImportDecls = flatMap(sortedImportGroups, importGroup => getExternalModuleName(importGroup[0].moduleSpecifier) ? coalesce(importGroup) : importGroup);

[amcasey]

Personally, I find that pulling a single line out of a method and moving it offscreen makes the method harder to read, but I agree that runtime type checks are undesirable (even though we're using JS).

Having said that, I found that I was fighting the type system for most of this change. For instance, I would have liked to type the parameter as ReadonlyArray<ImportDeclaration> | ReadonlyArray<ExportDeclaration>, but that caused even more problems.

[amcasey]

I've implemented it and it looks reasonable.

[ghost]

There's duplicate code in coalesceImports, could they share?

[amcasey]

I made getCategorizedImports shared and I find the result much uglier than what I started with. Did you have something specific in mind?

[ghost]

Sorry, not getCategorizedX, just the stableSort:

function sortSpecifiers<T extends ImportSpecifier | ExportSpecifier>(specifiers: ReadonlyArray<T>): ReadonlyArray<T> {
    return stableSort(specifiers, (s1, s2) =>
        compareIdentifiers(s1.propertyName || s1.name, s2.propertyName || s2.name) ||
        compareIdentifiers(s1.name, s2.name));
}

[amcasey]

Done.