Skip to content

Fix organizeImports with type-only imports #36807

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 14, 2020
Merged

Fix organizeImports with type-only imports #36807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
233 changes: 129 additions & 104 deletions src/services/organizeImports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,115 +174,134 @@ namespace ts.OrganizeImports {
return importGroup;
}

const { importWithoutClause, defaultImports, namespaceImports, namedImports } = getCategorizedImports(importGroup);
const { importWithoutClause, typeOnlyImports, regularImports } = getCategorizedImports(importGroup);

const coalescedImports: ImportDeclaration[] = [];

if (importWithoutClause) {
coalescedImports.push(importWithoutClause);
}

// Normally, we don't combine default and namespace imports, but it would be silly to
// produce two import declarations in this special case.
if (defaultImports.length === 1 && namespaceImports.length === 1 && namedImports.length === 0) {
// Add the namespace import to the existing default ImportDeclaration.
const defaultImport = defaultImports[0];
coalescedImports.push(
updateImportDeclarationAndClause(defaultImport, defaultImport.importClause!.name, namespaceImports[0].importClause!.namedBindings)); // TODO: GH#18217
for (const group of [regularImports, typeOnlyImports]) {
const isTypeOnly = group === typeOnlyImports;
const { defaultImports, namespaceImports, namedImports } = group;
// Normally, we don't combine default and namespace imports, but it would be silly to
// produce two import declarations in this special case.
if (!isTypeOnly && defaultImports.length === 1 && namespaceImports.length === 1 && namedImports.length === 0) {
// Add the namespace import to the existing default ImportDeclaration.
const defaultImport = defaultImports[0];
coalescedImports.push(
updateImportDeclarationAndClause(defaultImport, defaultImport.importClause!.name, namespaceImports[0].importClause!.namedBindings)); // TODO: GH#18217

return coalescedImports;
}
continue;
}

const sortedNamespaceImports = stableSort(namespaceImports, (i1, i2) =>
compareIdentifiers((i1.importClause!.namedBindings as NamespaceImport).name, (i2.importClause!.namedBindings as NamespaceImport).name)); // TODO: GH#18217
const sortedNamespaceImports = stableSort(namespaceImports, (i1, i2) =>
compareIdentifiers((i1.importClause!.namedBindings as NamespaceImport).name, (i2.importClause!.namedBindings as NamespaceImport).name)); // TODO: GH#18217

for (const namespaceImport of sortedNamespaceImports) {
// Drop the name, if any
coalescedImports.push(
updateImportDeclarationAndClause(namespaceImport, /*name*/ undefined, namespaceImport.importClause!.namedBindings)); // TODO: GH#18217
}
for (const namespaceImport of sortedNamespaceImports) {
// Drop the name, if any
coalescedImports.push(
updateImportDeclarationAndClause(namespaceImport, /*name*/ undefined, namespaceImport.importClause!.namedBindings)); // TODO: GH#18217
}

if (defaultImports.length === 0 && namedImports.length === 0) {
return coalescedImports;
}
if (defaultImports.length === 0 && namedImports.length === 0) {
continue;
}

let newDefaultImport: Identifier | undefined;
const newImportSpecifiers: ImportSpecifier[] = [];
if (defaultImports.length === 1) {
newDefaultImport = defaultImports[0].importClause!.name;
}
else {
for (const defaultImport of defaultImports) {
newImportSpecifiers.push(
createImportSpecifier(createIdentifier("default"), defaultImport.importClause!.name!)); // TODO: GH#18217
let newDefaultImport: Identifier | undefined;
const newImportSpecifiers: ImportSpecifier[] = [];
if (defaultImports.length === 1) {
newDefaultImport = defaultImports[0].importClause!.name;
}
else {
for (const defaultImport of defaultImports) {
newImportSpecifiers.push(
createImportSpecifier(createIdentifier("default"), defaultImport.importClause!.name!)); // TODO: GH#18217
}
}
}

newImportSpecifiers.push(...flatMap(namedImports, i => (i.importClause!.namedBindings as NamedImports).elements)); // TODO: GH#18217
newImportSpecifiers.push(...flatMap(namedImports, i => (i.importClause!.namedBindings as NamedImports).elements)); // TODO: GH#18217

const sortedImportSpecifiers = sortSpecifiers(newImportSpecifiers);
const sortedImportSpecifiers = sortSpecifiers(newImportSpecifiers);

const importDecl = defaultImports.length > 0
? defaultImports[0]
: namedImports[0];
const importDecl = defaultImports.length > 0
? defaultImports[0]
: namedImports[0];

const newNamedImports = sortedImportSpecifiers.length === 0
? newDefaultImport
? undefined
: createNamedImports(emptyArray)
: namedImports.length === 0
? createNamedImports(sortedImportSpecifiers)
: updateNamedImports(namedImports[0].importClause!.namedBindings as NamedImports, sortedImportSpecifiers); // TODO: GH#18217
const newNamedImports = sortedImportSpecifiers.length === 0
? newDefaultImport
? undefined
: createNamedImports(emptyArray)
: namedImports.length === 0
? createNamedImports(sortedImportSpecifiers)
: updateNamedImports(namedImports[0].importClause!.namedBindings as NamedImports, sortedImportSpecifiers); // TODO: GH#18217

coalescedImports.push(
updateImportDeclarationAndClause(importDecl, newDefaultImport, newNamedImports));
// Type-only imports are not allowed to combine
if (isTypeOnly && newDefaultImport && newNamedImports) {
coalescedImports.push(
updateImportDeclarationAndClause(importDecl, newDefaultImport, /*namedBindings*/ undefined));
coalescedImports.push(
updateImportDeclarationAndClause(namedImports[0] ?? importDecl, /*name*/ undefined, newNamedImports));
}
else {
coalescedImports.push(
updateImportDeclarationAndClause(importDecl, newDefaultImport, newNamedImports));
}
}

return coalescedImports;

/*
* Returns entire import declarations because they may already have been rewritten and
* may lack parent pointers. The desired parts can easily be recovered based on the
* categorization.
*
* NB: There may be overlap between `defaultImports` and `namespaceImports`/`namedImports`.
*/
function getCategorizedImports(importGroup: readonly ImportDeclaration[]) {
let importWithoutClause: ImportDeclaration | undefined;
const defaultImports: ImportDeclaration[] = [];
const namespaceImports: ImportDeclaration[] = [];
const namedImports: ImportDeclaration[] = [];

for (const importDeclaration of importGroup) {
if (importDeclaration.importClause === undefined) {
// Only the first such import is interesting - the others are redundant.
// Note: Unfortunately, we will lose trivia that was on this node.
importWithoutClause = importWithoutClause || importDeclaration;
continue;
}
}

const { name, namedBindings } = importDeclaration.importClause;
interface ImportGroup {
defaultImports: ImportDeclaration[];
namespaceImports: ImportDeclaration[];
namedImports: ImportDeclaration[];
}

if (name) {
defaultImports.push(importDeclaration);
}
/*
* Returns entire import declarations because they may already have been rewritten and
* may lack parent pointers. The desired parts can easily be recovered based on the
* categorization.
*
* NB: There may be overlap between `defaultImports` and `namespaceImports`/`namedImports`.
*/
function getCategorizedImports(importGroup: readonly ImportDeclaration[]) {
let importWithoutClause: ImportDeclaration | undefined;
const typeOnlyImports: ImportGroup = { defaultImports: [], namespaceImports: [], namedImports: [] };
const regularImports: ImportGroup = { defaultImports: [], namespaceImports: [], namedImports: [] };

for (const importDeclaration of importGroup) {
if (importDeclaration.importClause === undefined) {
// Only the first such import is interesting - the others are redundant.
// Note: Unfortunately, we will lose trivia that was on this node.
importWithoutClause = importWithoutClause || importDeclaration;
continue;
}

if (namedBindings) {
if (isNamespaceImport(namedBindings)) {
namespaceImports.push(importDeclaration);
}
else {
namedImports.push(importDeclaration);
}
}
const group = importDeclaration.importClause.isTypeOnly ? typeOnlyImports : regularImports;
const { name, namedBindings } = importDeclaration.importClause;

if (name) {
group.defaultImports.push(importDeclaration);
}

return {
importWithoutClause,
defaultImports,
namespaceImports,
namedImports,
};
if (namedBindings) {
if (isNamespaceImport(namedBindings)) {
group.namespaceImports.push(importDeclaration);
}
else {
group.namedImports.push(importDeclaration);
}
}
}

return {
importWithoutClause,
typeOnlyImports,
regularImports,
};
}

// Internal for testing
Expand All @@ -294,37 +313,38 @@ namespace ts.OrganizeImports {
return exportGroup;
}

const { exportWithoutClause, namedExports } = getCategorizedExports(exportGroup);
const { exportWithoutClause, namedExports, typeOnlyExports } = getCategorizedExports(exportGroup);

const coalescedExports: ExportDeclaration[] = [];

if (exportWithoutClause) {
coalescedExports.push(exportWithoutClause);
}

if (namedExports.length === 0) {
return coalescedExports;
for (const exportGroup of [namedExports, typeOnlyExports]) {
if (exportGroup.length === 0) {
continue;
}
const newExportSpecifiers: ExportSpecifier[] = [];
newExportSpecifiers.push(...flatMap(exportGroup, i => i.exportClause && isNamedExports(i.exportClause) ? i.exportClause.elements : emptyArray));

const sortedExportSpecifiers = sortSpecifiers(newExportSpecifiers);

const exportDecl = exportGroup[0];
coalescedExports.push(
updateExportDeclaration(
exportDecl,
exportDecl.decorators,
exportDecl.modifiers,
exportDecl.exportClause && (
isNamedExports(exportDecl.exportClause) ?
updateNamedExports(exportDecl.exportClause, sortedExportSpecifiers) :
updateNamespaceExport(exportDecl.exportClause, exportDecl.exportClause.name)
),
exportDecl.moduleSpecifier,
exportDecl.isTypeOnly));
}

const newExportSpecifiers: ExportSpecifier[] = [];
newExportSpecifiers.push(...flatMap(namedExports, i => i.exportClause && isNamedExports(i.exportClause) ? i.exportClause.elements : emptyArray));

const sortedExportSpecifiers = sortSpecifiers(newExportSpecifiers);

const exportDecl = namedExports[0];
coalescedExports.push(
updateExportDeclaration(
exportDecl,
exportDecl.decorators,
exportDecl.modifiers,
exportDecl.exportClause && (
isNamedExports(exportDecl.exportClause) ?
updateNamedExports(exportDecl.exportClause, sortedExportSpecifiers) :
updateNamespaceExport(exportDecl.exportClause, exportDecl.exportClause.name)
),
exportDecl.moduleSpecifier,
exportDecl.isTypeOnly));

return coalescedExports;

/*
Expand All @@ -335,13 +355,17 @@ namespace ts.OrganizeImports {
function getCategorizedExports(exportGroup: readonly ExportDeclaration[]) {
let exportWithoutClause: ExportDeclaration | undefined;
const namedExports: ExportDeclaration[] = [];
const typeOnlyExports: ExportDeclaration[] = [];

for (const exportDeclaration of exportGroup) {
if (exportDeclaration.exportClause === undefined) {
// Only the first such export is interesting - the others are redundant.
// Note: Unfortunately, we will lose trivia that was on this node.
exportWithoutClause = exportWithoutClause || exportDeclaration;
}
else if (exportDeclaration.isTypeOnly) {
typeOnlyExports.push(exportDeclaration);
}
else {
namedExports.push(exportDeclaration);
}
Expand All @@ -350,6 +374,7 @@ namespace ts.OrganizeImports {
return {
exportWithoutClause,
namedExports,
typeOnlyExports,
};
}
}
Expand Down
53 changes: 53 additions & 0 deletions src/testRunner/unittests/services/organizeImports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,28 @@ namespace ts {
const expectedCoalescedImports = sortedImports;
assertListEqual(actualCoalescedImports, expectedCoalescedImports);
});

it("Combine type-only imports separately from other imports", () => {
const sortedImports = parseImports(
`import type { x } from "lib";`,
`import type { y } from "lib";`,
`import { z } from "lib";`);
const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports);
const expectedCoalescedImports = parseImports(
`import { z } from "lib";`,
`import type { x, y } from "lib";`);
assertListEqual(actualCoalescedImports, expectedCoalescedImports);
});

it("Do not combine type-only default, namespace, or named imports with each other", () => {
const sortedImports = parseImports(
`import type { x } from "lib";`,
`import type * as y from "lib";`,
`import type z from "lib";`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why cant this mix with import type { x, default as z} from "lib just like normal import statement ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we decided it would be ambiguous what the "type" keyword applied to and forced explicitness.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could, but I’m not confident that’s desirable. My personal preference for readability would probably be what’s represented here in the test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewbranch Consider adding a comment about this: // Type-only imports are not allowed to combine (for readability). Maybe on the test too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably possible at a later pass, but seems reasonable to leave it the way it is now until we get further feedback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a stylistic choice in organizeImports, I don't think "allowed" is the right word - I definitely read that as a language restriction.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment in the implementation was actually referring to the language restriction, ignoring the possibility of rewriting the default as a named import. I’ll follow up to clarify.

const actualCoalescedImports = OrganizeImports.coalesceImports(sortedImports);
const expectedCoalescedImports = actualCoalescedImports;
assertListEqual(actualCoalescedImports, expectedCoalescedImports);
});
});

describe("Coalesce exports", () => {
Expand Down Expand Up @@ -240,6 +262,25 @@ namespace ts {
`export { x as a, y, z as b } from "lib";`);
assertListEqual(actualCoalescedExports, expectedCoalescedExports);
});

it("Keep type-only exports separate", () => {
const sortedExports = parseExports(
`export { x };`,
`export type { y };`);
const actualCoalescedExports = OrganizeImports.coalesceExports(sortedExports);
const expectedCoalescedExports = sortedExports;
assertListEqual(actualCoalescedExports, expectedCoalescedExports);
});

it("Combine type-only exports", () => {
const sortedExports = parseExports(
`export type { x };`,
`export type { y };`);
const actualCoalescedExports = OrganizeImports.coalesceExports(sortedExports);
const expectedCoalescedExports = parseExports(
`export type { x, y };`);
assertListEqual(actualCoalescedExports, expectedCoalescedExports);
});
});

describe("Baselines", () => {
Expand Down Expand Up @@ -425,6 +466,18 @@ D();
libFile);
/* eslint-enable no-template-curly-in-string */

testOrganizeImports("TypeOnly",
{
path: "/test.ts",
content: `
import { X } from "lib";
import type Y from "lib";
import { Z } from "lib";
import type { A, B } from "lib";

export { A, B, X, Y, Z };`
});

testOrganizeImports("CoalesceMultipleModules",
{
path: "/test.ts",
Expand Down
15 changes: 15 additions & 0 deletions tests/baselines/reference/organizeImports/TypeOnly.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// ==ORIGINAL==

import { X } from "lib";
import type Y from "lib";
import { Z } from "lib";
import type { A, B } from "lib";

export { A, B, X, Y, Z };
// ==ORGANIZED==

import { X, Z } from "lib";
import type Y from "lib";
import type { A, B } from "lib";

export { A, B, X, Y, Z };