Skip to content

Commit 9f666c1

Browse files
marcjmarcj
committed
fix(type-compiler): ensure typed imports are grouped and not too high up, but placed right in front of the origin import
This fixes various issues when import order are crucial.
1 parent 53215d1 commit 9f666c1

2 files changed

Lines changed: 81 additions & 36 deletions

File tree

‎packages/type-compiler/src/compiler.ts‎

Lines changed: 68 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ import ts, {
4444
InferTypeNode,
4545
InterfaceDeclaration,
4646
IntersectionTypeNode,
47+
isJSDocImportTag,
4748
JSDocImportTag,
4849
LiteralTypeNode,
4950
MappedTypeNode,
@@ -54,6 +55,7 @@ import ts, {
5455
ModuleExportName,
5556
NewExpression,
5657
Node,
58+
NodeArray,
5759
NodeFactory,
5860
ParseConfigHost,
5961
PropertyAccessExpression,
@@ -512,7 +514,8 @@ export class ReflectionTransformer implements CustomTransformer {
512514
*/
513515
protected compiledDeclarations = new Set<Node>();
514516

515-
protected addImports: { from: Expression, identifier: Identifier }[] = [];
517+
protected addImports: { importDeclaration: ImportDeclaration | JSDocImportTag, identifier: Identifier }[] = [];
518+
protected additionalImports = new Map<ImportDeclaration | JSDocImportTag, Statement>();
516519

517520
protected nodeConverter: NodeConverter;
518521
protected typeChecker?: TypeChecker;
@@ -653,6 +656,7 @@ export class ReflectionTransformer implements CustomTransformer {
653656
if ((sourceFile as any).deepkitTransformed) return sourceFile;
654657
this.embedAssignType = false;
655658
this.addImports = [];
659+
this.additionalImports.clear();
656660

657661
const start = Date.now();
658662
const configResolver = this.getConfigResolver(sourceFile);
@@ -961,40 +965,60 @@ export class ReflectionTransformer implements CustomTransformer {
961965

962966
if (this.addImports.length) {
963967
const handledIdentifier: string[] = [];
968+
// group by importDeclaration so that we have one `{...} per importDeclaration`
969+
const importMap = new Map<ImportDeclaration | JSDocImportTag, Identifier[]>();
964970
for (const imp of this.addImports) {
965971
if (handledIdentifier.includes(getIdentifierName(imp.identifier))) continue;
966972
handledIdentifier.push(getIdentifierName(imp.identifier));
973+
let arr = importMap.get(imp.importDeclaration);
974+
if (!arr) {
975+
arr = [];
976+
importMap.set(imp.importDeclaration, arr);
977+
}
978+
arr.push(imp.identifier);
979+
}
980+
981+
for (const [importDeclaration, identifiers] of importMap.entries()) {
982+
if (this.additionalImports.has(importDeclaration)) {
983+
throw new Error('Internal error: additional import already exists');
984+
}
967985
if (this.getModuleType() === 'cjs') {
968-
//var {identifier} = require('./bar')
969-
const test = this.f.createIdentifier(getIdentifierName(imp.identifier));
970-
const variable = this.f.createVariableStatement(undefined, this.f.createVariableDeclarationList([this.f.createVariableDeclaration(
971-
this.f.createObjectBindingPattern([this.f.createBindingElement(undefined, undefined, test)]),
972-
undefined, undefined,
973-
this.f.createCallExpression(this.f.createIdentifier('require'), undefined, [imp.from]),
974-
)], NodeFlags.Const));
986+
// var {a, b, c} = require('./bar')
987+
const varDeclaration = this.f.createVariableStatement(
988+
undefined,
989+
this.f.createVariableDeclarationList(
990+
[this.f.createVariableDeclaration(
991+
this.f.createObjectBindingPattern(identifiers.map(identifier => this.f.createBindingElement(
992+
undefined, undefined, identifier, undefined
993+
))),
994+
undefined,
995+
undefined,
996+
this.f.createCallExpression(this.f.createIdentifier("require"), undefined, [importDeclaration.moduleSpecifier])
997+
)],
998+
ts.NodeFlags.None
999+
)
1000+
);
1001+
9751002
const typeDeclWithComment = addSyntheticLeadingComment(
976-
variable,
1003+
varDeclaration,
9771004
SyntaxKind.MultiLineCommentTrivia,
9781005
'@ts-ignore',
9791006
true,
9801007
);
981-
newTopStatements.push(typeDeclWithComment);
1008+
this.additionalImports.set(importDeclaration, typeDeclWithComment);
9821009
} else {
983-
//import {identifier} from './bar.js'
984-
// import { identifier as identifier } is used to avoid automatic elision of imports (in angular builds for example)
985-
// that's probably a bit unstable.
986-
const specifier = this.f.createImportSpecifier(false, undefined, imp.identifier);
987-
const namedImports = this.f.createNamedImports([specifier]);
1010+
// import {a, b, c} from './bar.js'
1011+
const namedImports = this.f.createNamedImports(identifiers.map(identifier => this.f.createImportSpecifier(false, undefined, identifier)));
9881012
const importStatement = this.f.createImportDeclaration(undefined,
989-
this.f.createImportClause(false, undefined, namedImports), imp.from,
1013+
this.f.createImportClause(false, undefined, namedImports), importDeclaration.moduleSpecifier,
9901014
);
9911015
const typeDeclWithComment = addSyntheticLeadingComment(
9921016
importStatement,
9931017
SyntaxKind.MultiLineCommentTrivia,
9941018
'@ts-ignore',
9951019
true,
9961020
);
997-
newTopStatements.push(typeDeclWithComment);
1021+
this.additionalImports.set(importDeclaration, typeDeclWithComment);
9981022
}
9991023
}
10001024
}
@@ -1059,20 +1083,17 @@ export class ReflectionTransformer implements CustomTransformer {
10591083
);
10601084
}
10611085

1062-
if (newTopStatements.length) {
1063-
// we want to keep "use strict", or "use client", etc at the very top
1064-
const indexOfFirstLiteralExpression = this.sourceFile.statements.findIndex(v => isExpressionStatement(v) && isStringLiteral(v.expression));
1065-
1066-
const newStatements = indexOfFirstLiteralExpression === -1
1067-
? [...newTopStatements, ...this.sourceFile.statements]
1068-
: [
1069-
...this.sourceFile.statements.slice(0, indexOfFirstLiteralExpression + 1),
1070-
...newTopStatements,
1071-
...this.sourceFile.statements.slice(indexOfFirstLiteralExpression + 1),
1072-
];
1073-
this.sourceFile = this.f.updateSourceFile(this.sourceFile, newStatements);
1074-
// this.sourceFile = this.f.updateSourceFile(this.sourceFile, [...newTopStatements, ...this.sourceFile.statements]);
1075-
}
1086+
// we want to keep "use strict", or "use client", etc at the very top
1087+
const indexOfFirstLiteralExpression = this.sourceFile.statements.findIndex(v => isExpressionStatement(v) && isStringLiteral(v.expression));
1088+
1089+
const newStatements = indexOfFirstLiteralExpression === -1
1090+
? [...newTopStatements, ...this.attachAdditionalStatements(this.sourceFile.statements)]
1091+
: [
1092+
...this.sourceFile.statements.slice(0, indexOfFirstLiteralExpression + 1),
1093+
...newTopStatements,
1094+
...this.attachAdditionalStatements(this.sourceFile.statements.slice(indexOfFirstLiteralExpression + 1)),
1095+
];
1096+
this.sourceFile = this.f.updateSourceFile(this.sourceFile, newStatements);
10761097

10771098
// console.log(createPrinter().printNode(EmitHint.SourceFile, this.sourceFile, this.sourceFile));
10781099
const took = Date.now() - start;
@@ -1081,6 +1102,20 @@ export class ReflectionTransformer implements CustomTransformer {
10811102
return this.sourceFile;
10821103
}
10831104

1105+
attachAdditionalStatements(statements: NodeArray<Statement> | Statement[]): Statement[] {
1106+
const result: Statement[] = [];
1107+
for (const statement of statements) {
1108+
if (isImportDeclaration(statement) || isJSDocImportTag(statement)) {
1109+
const additional = this.additionalImports.get(statement);
1110+
if (additional) {
1111+
result.push(additional);
1112+
}
1113+
}
1114+
result.push(statement);
1115+
}
1116+
return result;
1117+
}
1118+
10841119
protected getModuleType(): 'cjs' | 'esm' {
10851120
if (this.compilerOptions.module === ts.ModuleKind.Node16 || this.compilerOptions.module === ts.ModuleKind.NodeNext) {
10861121
if (this.sourceFile.impliedNodeFormat === ts.ModuleKind.ESNext) {
@@ -2315,7 +2350,7 @@ export class ReflectionTransformer implements CustomTransformer {
23152350
return;
23162351
}
23172352

2318-
this.addImports.push({ identifier: runtimeTypeName, from: resolved.importDeclaration.moduleSpecifier });
2353+
this.addImports.push({ identifier: runtimeTypeName, importDeclaration: resolved.importDeclaration });
23192354
} else {
23202355
const reflection = this.getReflectionConfig(declarationSourceFile);
23212356
// if this is never, then its generally disabled for this file
@@ -2330,7 +2365,7 @@ export class ReflectionTransformer implements CustomTransformer {
23302365
return;
23312366
}
23322367

2333-
this.addImports.push({ identifier: runtimeTypeName, from: resolved.importDeclaration.moduleSpecifier });
2368+
this.addImports.push({ identifier: runtimeTypeName, importDeclaration: resolved.importDeclaration });
23342369
}
23352370
}
23362371
} else {

‎packages/type-compiler/tests/transform.spec.ts‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import * as ts from 'typescript';
22
import { createSourceFile, ScriptKind, ScriptTarget } from 'typescript';
33
import { expect, test } from '@jest/globals';
44
import { ReflectionTransformer } from '../src/compiler.js';
5-
import { transform, transpileAndRun } from './utils.js';
5+
import { transform } from './utils.js';
66

77
test('transform simple TS', () => {
88
const sourceFile = createSourceFile('app.ts', `
@@ -184,19 +184,29 @@ test('declaration file', () => {
184184
test('declaration file resolved export all', () => {
185185
const res = transform({
186186
'app.ts': `
187-
import { T } from './module';
187+
import 'import';
188+
import { T, T2 } from './module';
189+
import 'import2';
188190
typeOf<T>();
191+
typeOf<T2>();
189192
`,
190193
'module.d.ts': `
191194
export * from './module/types';
192195
`,
193196
'module/types.d.ts': `
194197
export type T = string;
198+
export type T2 = string;
195199
export type __ΩT = any[];
200+
export type __ΩT2 = any[];
196201
`
197202
});
198203

199-
expect(res['app.ts']).toContain('import { __ΩT } from \'./module');
204+
// It's important to not put the __ΩT import before other imports
205+
expect(res['app.ts']).toContain(`import 'import';
206+
/*@ts-ignore*/
207+
import { __ΩT, __ΩT2 } from './module';
208+
import { T, T2 } from './module';
209+
import 'import2';`);
200210
});
201211

202212
test('import typeOnly interface', () => {

0 commit comments

Comments
 (0)