Sort exports when organizeImports is run#24237
Conversation
Note that there's no attempt to remove unused exports. Fixes microsoft#23640
|
Just for you, @DanielRosenwasser. |
| assertListEqual(actualCoalescedExports, expectedCoalescedExports); | ||
| }); | ||
|
|
||
| it("Sort specifiers - case-insensitive", () => { |
There was a problem hiding this comment.
Seems like the test above this one is unnecessary?
|
|
||
| function parseExports(...exportStrings: string[]): ReadonlyArray<ExportDeclaration> { | ||
| const sourceFile = createSourceFile("a.ts", exportStrings.join("\n"), ScriptTarget.ES2015, /*setParentNodes*/ true, ScriptKind.TS); | ||
| const exports = filter(sourceFile.statements, isExportDeclaration); |
There was a problem hiding this comment.
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>;
}There was a problem hiding this comment.
I'm not sure I understand what benefit this provides.
| return changeTracker.getChanges(); | ||
|
|
||
| function organizeImportsWorker(oldImportDecls: ReadonlyArray<ImportDeclaration>) { | ||
| function organizeImportsWorker(oldImportDecls: ReadonlyArray<ImportDeclaration | ExportDeclaration>) { |
There was a problem hiding this comment.
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);There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've implemented it and it looks reasonable.
| const newExportSpecifiers: ExportSpecifier[] = []; | ||
| newExportSpecifiers.push(...flatMap(namedExports, i => (i.exportClause).elements)); | ||
|
|
||
| const sortedExportSpecifiers = stableSort(newExportSpecifiers, (s1, s2) => |
There was a problem hiding this comment.
There's duplicate code in coalesceImports, could they share?
There was a problem hiding this comment.
I made getCategorizedImports shared and I find the result much uglier than what I started with. Did you have something specific in mind?
There was a problem hiding this comment.
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));
}
Note that there's no attempt to remove unused exports. I also didn't move the existing import tests into a corresponding
describefor consistency because I wanted to avoid blowing up the diff.Fixes #23640