Skip to content

Commit af7a8a5

Browse files
committed
fix: Clarify legacy mode semantics and add meta-validation tests
- Update legacy mode documentation to accurately describe all behaviors: * Within-group sorting bug * Blank line preservation * Merge timing (before removeTrailingIndex) * Type-only import merging - Add manifest validation tests (package.json correctness) - Add file structure validation tests (no duplicates, correct model) - Fix misleading test message (attributes ARE preserved, not stripped) - All 262 main tests + 162 comparison tests passing
1 parent 3af3ba8 commit af7a8a5

6 files changed

Lines changed: 254 additions & 5 deletions

File tree

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@
149149
"miniTypescriptHero.imports.legacyMode": {
150150
"type": "boolean",
151151
"default": false,
152-
"description": "Enable full legacy mode for 100% compatibility with old TypeScript Hero extension. When true: replicates ALL old behaviors (within-group sorting bug, preserve blank lines, no merging). When false: modern best practices (correct sorting, 1 blank line, always merge). Migrated users get 'true' automatically.",
152+
"description": "Enable full legacy mode for 100% compatibility with old TypeScript Hero extension. When true: replicates ALL old behaviors (within-group sorting bug, preserve blank lines, merge-before-removeTrailingIndex timing, type-only merge). When false: modern best practices (correct sorting, 1 blank line, correct merge timing, type-only separation). Migrated users get 'true' automatically.",
153153
"scope": "resource"
154154
},
155155
"miniTypescriptHero.imports.blankLinesAfterImports": {

src/configuration/imports-config.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,10 +83,14 @@ export class ImportsConfig {
8383
* When true, enables ALL legacy behaviors:
8484
* - Within-group sorting: Always sorts by library name (ignores disableImportsSorting/organizeSortsByFirstSpecifier)
8585
* - Blank lines: Uses 'preserve' mode (keeps existing blank lines from source)
86+
* - Merge timing: When mergeImportsFromSameModule is true, merges BEFORE removeTrailingIndex (matches old bug where './lib/index' and './lib' stay separate)
87+
* - Type-only merging: Strips 'import type' keywords and allows merging type-only with value imports (old behavior)
8688
*
8789
* When false (default), uses modern best practices:
8890
* - Sorting: Respects all sorting configs correctly
8991
* - Blank lines: Exactly 1 blank line (Google/ESLint/Prettier standard)
92+
* - Merge timing: Removes '/index' FIRST, then merges (so './lib/index' and './lib' DO merge)
93+
* - Type-only separation: Keeps 'import type' keywords and prevents merging type-only with value imports (TS 3.8+ semantics)
9094
*
9195
* Default: false (new users get modern behavior)
9296
* Migrated users: Automatically set to true for 100% backward compatibility

src/configuration/settings-migration.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -110,9 +110,10 @@ async function performMigration(): Promise<number> {
110110
// For migrated users: Enable legacyMode for 100% backward compatibility
111111
if (migratedCount > 0) {
112112
// Simply enable legacyMode: true to replicate ALL old behaviors:
113-
// - Within-group sorting bug (always sorts by library name)
114-
// - Blank line preservation (keeps existing blank lines)
115-
// - removeTrailingIndex applied after merging (old bug)
113+
// - Within-group sorting bug (always sorts by library name, ignores disableImportsSorting/organizeSortsByFirstSpecifier)
114+
// - Blank line preservation (uses 'preserve' mode, keeps existing blank lines from source)
115+
// - Merge timing: When mergeImportsFromSameModule is true, merges BEFORE removeTrailingIndex (matches old bug)
116+
// - Type-only merging: Strips 'import type' keywords and allows merging type-only with value imports (old behavior)
116117
const legacyModeInspect = newConfig.inspect('legacyMode');
117118
if (legacyModeInspect?.globalValue === undefined &&
118119
legacyModeInspect?.workspaceValue === undefined &&

src/test/file-structure.test.ts

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
/**
2+
* File structure validation tests
3+
*
4+
* These tests ensure there are no duplicate core files that could cause
5+
* conflicting behavior at runtime.
6+
*/
7+
8+
import * as assert from 'assert';
9+
import * as fs from 'fs';
10+
import * as path from 'path';
11+
12+
suite('File Structure Validation', () => {
13+
14+
test('exactly one import-organizer.ts exists', () => {
15+
// When tests run from out/test/, we need to go up to project root then into src
16+
const projectRoot = path.resolve(__dirname, '../..');
17+
const srcDir = path.join(projectRoot, 'src/imports');
18+
const files = findFilesRecursive(srcDir, 'import-organizer.ts');
19+
20+
assert.strictEqual(
21+
files.length,
22+
1,
23+
`Expected exactly 1 import-organizer.ts, found ${files.length}: ${files.join(', ')}`
24+
);
25+
});
26+
27+
test('exactly one import-types.ts exists', () => {
28+
// When tests run from out/test/, we need to go up to project root then into src
29+
const projectRoot = path.resolve(__dirname, '../..');
30+
const srcDir = path.join(projectRoot, 'src/imports');
31+
const files = findFilesRecursive(srcDir, 'import-types.ts');
32+
33+
assert.strictEqual(
34+
files.length,
35+
1,
36+
`Expected exactly 1 import-types.ts, found ${files.length}: ${files.join(', ')}`
37+
);
38+
});
39+
40+
test('import-types.ts supports attributes', () => {
41+
const projectRoot = path.resolve(__dirname, '../..');
42+
const srcDir = path.join(projectRoot, 'src/imports');
43+
const files = findFilesRecursive(srcDir, 'import-types.ts');
44+
45+
assert.strictEqual(files.length, 1, 'Should have exactly one import-types.ts');
46+
47+
const content = fs.readFileSync(files[0], 'utf8');
48+
49+
// Check for attributes field in the Import interface
50+
assert.ok(
51+
/attributes\?:\s*string/.test(content),
52+
'import-types.ts must have attributes?: string field'
53+
);
54+
55+
// Check for isTypeOnly in NamedImport
56+
assert.ok(
57+
/isTypeOnly:\s*boolean/.test(content),
58+
'import-types.ts must have isTypeOnly: boolean field'
59+
);
60+
61+
// Check for specifier comments
62+
assert.ok(
63+
/leadingComment\?:\s*string/.test(content),
64+
'import-types.ts must have leadingComment?: string field'
65+
);
66+
67+
assert.ok(
68+
/trailingComment\?:\s*string/.test(content),
69+
'import-types.ts must have trailingComment?: string field'
70+
);
71+
});
72+
73+
test('import-organizer.ts does not register old command alias', () => {
74+
const projectRoot = path.resolve(__dirname, '../..');
75+
const srcDir = path.join(projectRoot, 'src/imports');
76+
const files = findFilesRecursive(srcDir, 'import-organizer.ts');
77+
78+
assert.strictEqual(files.length, 1, 'Should have exactly one import-organizer.ts');
79+
80+
const content = fs.readFileSync(files[0], 'utf8');
81+
82+
// Check that it does NOT register the old command
83+
assert.ok(
84+
!content.includes("'typescriptHero.imports.organize'"),
85+
'import-organizer.ts must NOT register the old typescriptHero.imports.organize command'
86+
);
87+
88+
// Check that it DOES register the new command
89+
assert.ok(
90+
content.includes("'miniTypescriptHero.imports.organize'"),
91+
'import-organizer.ts must register miniTypescriptHero.imports.organize command'
92+
);
93+
});
94+
95+
test('no conflicting source files exist', () => {
96+
const projectRoot = path.resolve(__dirname, '../..');
97+
const srcDir = path.join(projectRoot, 'src');
98+
99+
// These are the canonical files that must be unique
100+
const coreFiles = [
101+
'imports/import-organizer.ts',
102+
'imports/import-types.ts',
103+
'imports/import-manager.ts',
104+
'configuration/imports-config.ts',
105+
];
106+
107+
for (const relativePath of coreFiles) {
108+
const fullPath = path.join(srcDir, relativePath);
109+
assert.ok(
110+
fs.existsSync(fullPath),
111+
`Core file must exist: ${relativePath}`
112+
);
113+
114+
// Check that no duplicates exist anywhere else
115+
const filename = path.basename(relativePath);
116+
const allMatches = findFilesRecursive(srcDir, filename);
117+
118+
assert.strictEqual(
119+
allMatches.length,
120+
1,
121+
`Found multiple copies of ${filename}: ${allMatches.join(', ')}`
122+
);
123+
}
124+
});
125+
});
126+
127+
/**
128+
* Recursively find all files with a given name in a directory.
129+
*/
130+
function findFilesRecursive(dir: string, filename: string): string[] {
131+
const results: string[] = [];
132+
133+
if (!fs.existsSync(dir)) {
134+
return results;
135+
}
136+
137+
const entries = fs.readdirSync(dir, { withFileTypes: true });
138+
139+
for (const entry of entries) {
140+
const fullPath = path.join(dir, entry.name);
141+
142+
if (entry.isDirectory()) {
143+
// Skip node_modules and test directories
144+
if (entry.name !== 'node_modules' && entry.name !== 'test' && entry.name !== 'out') {
145+
results.push(...findFilesRecursive(fullPath, filename));
146+
}
147+
} else if (entry.isFile() && entry.name === filename) {
148+
results.push(fullPath);
149+
}
150+
}
151+
152+
return results;
153+
}

src/test/import-manager.edge-cases.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ const y = data;
163163
await applyEditsToDocument(doc, edits);
164164

165165
const result = doc.getText();
166-
assert.strictEqual(result, expected, 'Import with attributes stripped by ts-morph (known limitation)');
166+
assert.strictEqual(result, expected, 'Import attributes must be preserved');
167167
} finally {
168168
await deleteTempDocument(doc);
169169
}
Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
/**
2+
* Manifest validation tests
3+
*
4+
* These tests ensure the package.json manifest is correctly configured
5+
* and doesn't conflict with the old TypeScript Hero extension.
6+
*/
7+
8+
import * as assert from 'assert';
9+
import * as fs from 'fs';
10+
import * as path from 'path';
11+
12+
suite('Manifest Validation', () => {
13+
14+
let packageJson: any;
15+
16+
suiteSetup(() => {
17+
const packagePath = path.resolve(__dirname, '../../package.json');
18+
const content = fs.readFileSync(packagePath, 'utf8');
19+
packageJson = JSON.parse(content);
20+
});
21+
22+
test('contributes only miniTypescriptHero.* commands', () => {
23+
const commands = (packageJson.contributes?.commands ?? []).map((c: any) => c.command);
24+
25+
// All commands must start with miniTypescriptHero
26+
assert.ok(
27+
commands.every((c: string) => c.startsWith('miniTypescriptHero.')),
28+
`All commands must start with 'miniTypescriptHero.', found: ${commands.join(', ')}`
29+
);
30+
31+
// No commands should start with typescriptHero (old extension)
32+
assert.ok(
33+
commands.every((c: string) => !c.startsWith('typescriptHero.')),
34+
`No commands should start with 'typescriptHero.', found: ${commands.join(', ')}`
35+
);
36+
37+
// Should have at least the organize command
38+
assert.ok(
39+
commands.includes('miniTypescriptHero.imports.organize'),
40+
'Must include miniTypescriptHero.imports.organize command'
41+
);
42+
});
43+
44+
test('all configuration properties start with miniTypescriptHero.imports.*', () => {
45+
const configProps = Object.keys(packageJson.contributes?.configuration?.properties ?? {});
46+
47+
assert.ok(
48+
configProps.every(k => k.startsWith('miniTypescriptHero.imports.')),
49+
`All settings must start with 'miniTypescriptHero.imports.*', found: ${configProps.join(', ')}`
50+
);
51+
52+
// Verify no old extension settings
53+
assert.ok(
54+
configProps.every(k => !k.startsWith('typescriptHero.')),
55+
`No settings should start with 'typescriptHero.*', found: ${configProps.join(', ')}`
56+
);
57+
});
58+
59+
test('has correct extension metadata', () => {
60+
assert.strictEqual(packageJson.name, 'mini-typescript-hero', 'Extension name must be mini-typescript-hero');
61+
assert.strictEqual(packageJson.displayName, 'Mini TypeScript Hero', 'Display name must be Mini TypeScript Hero');
62+
assert.strictEqual(packageJson.publisher, 'angular-schule', 'Publisher must be angular-schule');
63+
});
64+
65+
test('ignoredFromRemoval has correct default', () => {
66+
const ignoredFromRemoval = packageJson.contributes?.configuration?.properties?.['miniTypescriptHero.imports.ignoredFromRemoval'];
67+
assert.deepStrictEqual(
68+
ignoredFromRemoval?.default,
69+
['react'],
70+
'Default for ignoredFromRemoval must be ["react"]'
71+
);
72+
});
73+
74+
test('legacyMode defaults to false for new users', () => {
75+
const legacyMode = packageJson.contributes?.configuration?.properties?.['miniTypescriptHero.imports.legacyMode'];
76+
assert.strictEqual(
77+
legacyMode?.default,
78+
false,
79+
'Default for legacyMode must be false (new users get modern behavior)'
80+
);
81+
});
82+
83+
test('mergeImportsFromSameModule defaults to true', () => {
84+
const merge = packageJson.contributes?.configuration?.properties?.['miniTypescriptHero.imports.mergeImportsFromSameModule'];
85+
assert.strictEqual(
86+
merge?.default,
87+
true,
88+
'Default for mergeImportsFromSameModule must be true (modern behavior)'
89+
);
90+
});
91+
});

0 commit comments

Comments
 (0)