Skip to content

Commit

Permalink
Properly elide or preserve empty imports/exports (#810)
Browse files Browse the repository at this point in the history 
Fixes #801

Sucrase previously had a somewhat unified approach to import/export elision, but
this caused an issue with `import {}` statements, which should be removed in
TypeScript, but not in Flow or with neither transform enabled. Now, we plumb the
full configuration to both transforms and use the configuration to decide which
strategy is relevant.
  • Loading branch information
@alangpierce
alangpierce committed Jul 13, 2023
1 parent 7bd7ca3 commit a924d84
Show file tree
Hide file tree
Showing 6 changed files with 169 additions and 27 deletions.
45 changes: 35 additions & 10 deletions src/transformers/CJSImportTransformer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export default class CJSImportTransformer extends Transformer {
readonly enableLegacyBabel5ModuleInterop: boolean,
readonly enableLegacyTypeScriptModuleInterop: boolean,
readonly isTypeScriptTransformEnabled: boolean,
readonly isFlowTransformEnabled: boolean,
readonly preserveDynamicImport: boolean,
) {
super();
Expand Down Expand Up @@ -142,9 +143,8 @@ export default class CJSImportTransformer extends Transformer {
return;
}

const wasOnlyTypes = this.removeImportAndDetectIfType();

if (wasOnlyTypes) {
const shouldElideImport = this.removeImportAndDetectIfShouldElide();
if (shouldElideImport) {
this.tokens.removeToken();
} else {
const path = this.tokens.stringValue();
Expand All @@ -158,12 +158,23 @@ export default class CJSImportTransformer extends Transformer {
}

/**
* Erase this import, and return true if it was either of the form "import type" or contained only
* "type" named imports. Such imports should not even do a side-effect import.
* Erase this import (since any CJS output would be completely different), and
* return true if this import is should be elided due to being a type-only
* import. Such imports will not be emitted at all to avoid side effects.
*
* Import elision only happens with the TypeScript or Flow transforms enabled.
*
* TODO: This function has some awkward overlap with
* CJSImportProcessor.pruneTypeOnlyImports , and the two should be unified.
* That function handles TypeScript implicit import name elision, and removes
* an import if all typical imported names (without `type`) are removed due
* to being type-only imports. This function handles Flow import removal and
* properly distinguishes `import 'foo'` from `import {} from 'foo'` for TS
* purposes.
*
* The position should end at the import string.
*/
private removeImportAndDetectIfType(): boolean {
private removeImportAndDetectIfShouldElide(): boolean {
this.tokens.removeInitialToken();
if (
this.tokens.matchesContextual(ContextualKeyword._type) &&
Expand All @@ -187,24 +198,38 @@ export default class CJSImportTransformer extends Transformer {
return false;
}

let foundNonType = false;
let foundNonTypeImport = false;
let foundAnyNamedImport = false;
while (!this.tokens.matches1(tt.string)) {
// Check if any named imports are of the form "foo" or "foo as bar", with
// no leading "type".
if ((!foundNonType && this.tokens.matches1(tt.braceL)) || this.tokens.matches1(tt.comma)) {
if (
(!foundNonTypeImport && this.tokens.matches1(tt.braceL)) ||
this.tokens.matches1(tt.comma)
) {
this.tokens.removeToken();
if (!this.tokens.matches1(tt.braceR)) {
foundAnyNamedImport = true;
}
if (
this.tokens.matches2(tt.name, tt.comma) ||
this.tokens.matches2(tt.name, tt.braceR) ||
this.tokens.matches4(tt.name, tt.name, tt.name, tt.comma) ||
this.tokens.matches4(tt.name, tt.name, tt.name, tt.braceR)
) {
foundNonType = true;
foundNonTypeImport = true;
}
}
this.tokens.removeToken();
}
return !foundNonType;
if (this.isTypeScriptTransformEnabled) {
return !foundNonTypeImport;
} else if (this.isFlowTransformEnabled) {
// In Flow, unlike TS, `import {} from 'foo';` preserves the import.
return foundAnyNamedImport && !foundNonTypeImport;
} else {
return false;
}
}

private removeRemainingImport(): void {
Expand Down
26 changes: 20 additions & 6 deletions src/transformers/ESMImportTransformer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export default class ESMImportTransformer extends Transformer {
readonly helperManager: HelperManager,
readonly reactHotLoaderTransformer: ReactHotLoaderTransformer | null,
readonly isTypeScriptTransformEnabled: boolean,
readonly isFlowTransformEnabled: boolean,
options: Options,
) {
super();
Expand Down Expand Up @@ -128,7 +129,7 @@ export default class ESMImportTransformer extends Transformer {

private processImportEquals(): boolean {
const importName = this.tokens.identifierNameAtIndex(this.tokens.currentIndex() + 1);
if (this.isTypeName(importName)) {
if (this.shouldAutomaticallyElideImportedName(importName)) {
// If this name is only used as a type, elide the whole import.
elideImportEquals(this.tokens);
} else if (this.injectCreateRequireForImportRequire) {
Expand Down Expand Up @@ -203,10 +204,12 @@ export default class ESMImportTransformer extends Transformer {
}

let foundNonTypeImport = false;
let foundAnyNamedImport = false;
let needsComma = false;

// Handle default import.
if (this.tokens.matches1(tt.name)) {
if (this.isTypeName(this.tokens.identifierName())) {
if (this.shouldAutomaticallyElideImportedName(this.tokens.identifierName())) {
this.tokens.removeToken();
if (this.tokens.matches1(tt.comma)) {
this.tokens.removeToken();
Expand All @@ -230,7 +233,7 @@ export default class ESMImportTransformer extends Transformer {
}

if (this.tokens.matches1(tt.star)) {
if (this.isTypeName(this.tokens.identifierNameAtRelativeIndex(2))) {
if (this.shouldAutomaticallyElideImportedName(this.tokens.identifierNameAtRelativeIndex(2))) {
this.tokens.removeToken();
this.tokens.removeToken();
this.tokens.removeToken();
Expand All @@ -249,8 +252,12 @@ export default class ESMImportTransformer extends Transformer {
}
this.tokens.copyToken();
while (!this.tokens.matches1(tt.braceR)) {
foundAnyNamedImport = true;
const specifierInfo = getImportExportSpecifierInfo(this.tokens);
if (specifierInfo.isType || this.isTypeName(specifierInfo.rightName)) {
if (
specifierInfo.isType ||
this.shouldAutomaticallyElideImportedName(specifierInfo.rightName)
) {
while (this.tokens.currentIndex() < specifierInfo.endIndex) {
this.tokens.removeToken();
}
Expand All @@ -270,10 +277,17 @@ export default class ESMImportTransformer extends Transformer {
this.tokens.copyExpectedToken(tt.braceR);
}

return !foundNonTypeImport;
if (this.isTypeScriptTransformEnabled) {
return !foundNonTypeImport;
} else if (this.isFlowTransformEnabled) {
// In Flow, unlike TS, `import {} from 'foo';` preserves the import.
return foundAnyNamedImport && !foundNonTypeImport;
} else {
return false;
}
}

private isTypeName(name: string): boolean {
private shouldAutomaticallyElideImportedName(name: string): boolean {
return this.isTypeScriptTransformEnabled && !this.nonTypeIdentifiers.has(name);
}

Expand Down
2 changes: 2 additions & 0 deletions src/transformers/RootTransformer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ export default class RootTransformer {
enableLegacyBabel5ModuleInterop,
Boolean(options.enableLegacyTypeScriptModuleInterop),
transforms.includes("typescript"),
transforms.includes("flow"),
Boolean(options.preserveDynamicImport),
),
);
Expand All @@ -106,6 +107,7 @@ export default class RootTransformer {
this.helperManager,
reactHotLoaderTransformer,
transforms.includes("typescript"),
transforms.includes("flow"),
options,
),
);
Expand Down
48 changes: 48 additions & 0 deletions test/flow-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,50 @@ describe("transform flow", () => {
);
});

it("preserves import {} statements when targeting CJS", () => {
assertFlowResult(
`
import {} from 'a';
`,
`"use strict";
require('a');
`,
);
});

it("preserves import {} statements when targeting ESM", () => {
assertFlowESMResult(
`
import {} from 'a';
`,
`
import {} from 'a';
`,
);
});

it("preserves re-export {} statements when targeting CJS", () => {
assertFlowResult(
`
export {} from 'a';
`,
`"use strict";${ESMODULE_PREFIX}
require('a');
`,
);
});

it("preserves re-export {} statements when targeting ESM", () => {
assertFlowESMResult(
`
export {} from 'a';
`,
`
export {} from 'a';
`,
);
});

it("does not mistake ? in types for a ternary operator", () => {
assertFlowResult(
`
Expand Down Expand Up @@ -231,10 +275,14 @@ describe("transform flow", () => {
`
import a, {type n as b, m as c, type d} from './e';
import type f from './g';
import {type h} from './i';
import j, {} from './k';
`,
`
import a, { m as c,} from './e';
import j, {} from './k';
`,
);
});
Expand Down
59 changes: 48 additions & 11 deletions test/imports-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -373,17 +373,6 @@ return obj && obj.__esModule ? obj : { default: obj }; }
);
});

it("allows an import statement with no import bindings", () => {
assertResult(
`
import {} from 'moduleName';
`,
`"use strict";
`,
);
});

it("handles trailing commas in named imports", () => {
assertResult(
`
Expand Down Expand Up @@ -1526,6 +1515,54 @@ module.exports = exports.default;
);
});

it("does not elide `import {}` with TS transform disabled when targeting ESM", () => {
assertResult(
`
import {} from './a';
`,
`
import {} from './a';
`,
{transforms: []},
);
});

it("does not elide `import {}` with TS transform disabled when targeting CJS", () => {
assertResult(
`
import {} from './a';
`,
`"use strict";
require('./a');
`,
{transforms: ["imports"]},
);
});

it("does not elide `export {}` with TS transform disabled when targeting ESM", () => {
assertResult(
`
export {} from './a';
`,
`
export {} from './a';
`,
{transforms: []},
);
});

it("does not elide `export {}` with TS transform disabled when targeting CJS", () => {
assertResult(
`
export {} from './a';
`,
`"use strict";${ESMODULE_PREFIX}
require('./a');
`,
{transforms: ["imports"]},
);
});

it("implements basic live bindings", () => {
assertMultiFileOutput(
{
Expand Down
16 changes: 16 additions & 0 deletions test/typescript-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3804,4 +3804,20 @@ describe("typescript transform", () => {
},
);
});

it("removes `import {}` statements when targeting TypeScript", () => {
assertTypeScriptImportResult(
`
import {} from 'a';
`,
{
expectedESMResult: `
`,
expectedCJSResult: `"use strict";
`,
},
);
});
});

0 comments on commit a924d84

Please sign in to comment.