fix: preserve statements between organized imports

src/organize.ts

@@ -3,39 +3,59 @@ import { printNode } from "./printer.ts";
 
 export function organizeImports(content: string, filePath: string): string {
   const sourceFile = ts.createSourceFile(filePath, content, ts.ScriptTarget.Latest);
-  const groups = new Map<string, ts.ImportDeclaration[]>();
-  let firstPos = -1;
-  let lastEnd = -1;
+  const groups: ts.ImportDeclaration[][] = [];
+  const adjacentGroups = new Map<string, ts.ImportDeclaration[]>();
 
   for (const stmt of sourceFile.statements) {
-    if (!ts.isImportDeclaration(stmt) || !ts.isStringLiteral(stmt.moduleSpecifier)) continue;
+    if (!ts.isImportDeclaration(stmt) || !ts.isStringLiteral(stmt.moduleSpecifier)) {
+      adjacentGroups.clear();
+      continue;
+    }
     const specifier = stmt.moduleSpecifier.text;
-    const group = groups.get(specifier);
+    const group = adjacentGroups.get(specifier);
     if (group) group.push(stmt);
-    else groups.set(specifier, [stmt]);
-    if (firstPos === -1) firstPos = stmt.getFullStart();
-    lastEnd = stmt.end;
+    else {
+      const imports = [stmt];
+      adjacentGroups.set(specifier, imports);
+      groups.push(imports);
+    }
   }
 
   let needsOrganize = false;
-  for (const group of groups.values()) {
+  for (const group of groups) {
     if (group.length > 1) {
       needsOrganize = true;
       break;
     }
   }
   if (!needsOrganize) return content;
 
-  const organized: string[] = [];
-  for (const group of groups.values()) {
+  const edits: { start: number; end: number; text: string }[] = [];
+  for (const group of groups) {
+    if (group.length < 2) continue;
+    if (
+      group.some((imp) =>
+        /@ts-(?:expect-error|ignore)\b/.test(content.slice(imp.getFullStart(), imp.getStart(sourceFile))),
+      )
+    ) {
+      continue;
+    }
+    const organized: string[] = [];
     // @ts-expect-error testCoalesceImports internal fn
     for (const imp of ts.OrganizeImports.testCoalesceImports(group, false, sourceFile, {})) {
+      // Leading and trailing comments remain outside the replacement ranges.
+      ts.setEmitFlags(imp, ts.EmitFlags.NoLeadingComments | ts.EmitFlags.NoTrailingComments);
       organized.push(printNode(imp, sourceFile));
     }
+    for (const [index, imp] of group.entries()) {
+      edits.push({ start: imp.getStart(sourceFile), end: imp.end, text: index === 0 ? organized.join("\n") : "" });
+    }
   }
 
-  let endPos = lastEnd;
-  while (endPos < content.length && (content[endPos] === "\n" || content[endPos] === "\r")) endPos++;
+  // Only replace imports, leaving interleaved statements and trivia intact.
+  for (const { start, end, text } of edits.sort((a, b) => b.start - a.start)) {
+    content = content.slice(0, start) + text + content.slice(end);
+  }
 
-  return content.slice(0, firstPos) + organized.join("\n") + "\n" + content.slice(endPos);
+  return content;
 }

test/integration/organize.test.ts

@@ -0,0 +1,89 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { execFile } from "node:child_process";
+import { mkdtemp, readFile, realpath, rm, stat, writeFile } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { pathToFileURL } from "node:url";
+import { promisify } from "node:util";
+import { unbarrelify } from "../../src/index.ts";
+
+const exec = promisify(execFile);
+
+test("organizing preserves statement boundaries around imports without semicolons", async (t) => {
+  const dir = await realpath(await mkdtemp(join(tmpdir(), "unbarrelify-organize-asi-")));
+  t.after(() => rm(dir, { recursive: true }));
+  await writeFile(join(dir, "value.ts"), "export const foo = 1, bar = 2;\n");
+  await writeFile(join(dir, "barrel.ts"), 'export { foo } from "./value.ts";\n');
+  const consumer = join(dir, "consumer.ts");
+  await writeFile(
+    consumer,
+    `import { foo } from "./barrel.ts";
+console.log(foo)
+import { bar } from "./value.ts";
+(() => console.log(bar))();
+`,
+  );
+  const before = await exec(process.execPath, [consumer], { timeout: 10_000 });
+  assert.equal(before.stdout, "1\n2\n");
+  await unbarrelify({ cwd: dir, files: ["*.ts"], ext: ".ts", write: true, organizeImports: true });
+  const after = await exec(process.execPath, [consumer], { timeout: 10_000 });
+  assert.equal(after.stdout, "1\n2\n");
+});
+
+test("organizing through the public API preserves exports and effect order", async (t) => {
+  const dir = await realpath(await mkdtemp(join(tmpdir(), "unbarrelify-organize-")));
+  const identity = await stat(dir);
+  t.after(async () => {
+    const current = await stat(dir);
+    assert.equal(current.dev, identity.dev);
+    assert.equal(current.ino, identity.ino);
+    await rm(dir, { recursive: true });
+  });
+
+  const consumerPath = join(dir, "consumer.ts");
+  await writeFile(join(dir, "value.ts"), 'console.log("value");\nexport const foo = 1;\nexport const bar = 2;\n');
+  await writeFile(join(dir, "barrel.ts"), 'export { foo } from "./value.ts";\n');
+  await writeFile(join(dir, "effect.ts"), 'console.log("effect");\n');
+  await writeFile(
+    consumerPath,
+    `#!/usr/bin/env node
+"use client";
+// Keep the public value.
+import { foo } from "./barrel.ts";
+export const saved = foo;
+console.log("before", saved);
+import "./effect.ts";
+// Keep the calculation between imports.
+function sum(a: number, b: number) { return a + b; }
+import { bar } from "./value.ts";
+console.log("after", saved, bar);
+export const total = sum(saved, bar);
+`,
+  );
+
+  const script = `import { saved, total } from ${JSON.stringify(pathToFileURL(consumerPath).href)};
+console.log(JSON.stringify({ saved, total }));`;
+  const expected = 'value\neffect\nbefore 1\nafter 1 2\n{"saved":1,"total":3}\n';
+  const before = await exec(process.execPath, ["--input-type=module", "-e", script], { timeout: 10_000 });
+  assert.equal(before.stdout, expected);
+
+  const result = await unbarrelify({
+    cwd: dir,
+    files: ["*.ts"],
+    ext: ".ts",
+    write: true,
+    organizeImports: true,
+  });
+  assert.deepEqual(result.untraceableImports, []);
+  assert.ok(result.modified.includes(consumerPath));
+  const after = await exec(process.execPath, ["--input-type=module", "-e", script], { timeout: 10_000 });
+  assert.equal(after.stdout, expected);
+
+  const content = await readFile(consumerPath, "utf-8");
+  assert.ok(content.startsWith('#!/usr/bin/env node\n"use client";\n'));
+  assert.ok(content.includes("// Keep the public value."));
+  assert.ok(content.includes("// Keep the calculation between imports."));
+  assert.ok(!content.includes("./barrel.ts"));
+  assert.equal((content.match(/from "\.\/value\.ts"/g) || []).length, 2);
+});

test/unit/organize.test.ts

@@ -0,0 +1,168 @@
+import { test, describe } from "node:test";
+import assert from "node:assert/strict";
+import { mkdtemp, rm, writeFile } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import ts from "typescript";
+import { organizeImports } from "../../src/organize.ts";
+
+describe("organizeImports", () => {
+  test("keeps compiler suppression directives attached to their imports", async (t) => {
+    const dir = await mkdtemp(join(tmpdir(), "unbarrelify-import-directives-"));
+    t.after(() => rm(dir, { recursive: true }));
+    const file = join(dir, "consumer.ts");
+    const content = `// @ts-expect-error This JavaScript module has no declarations.
+import { foo } from "./legacy.js";
+// @ts-expect-error This JavaScript module has no declarations.
+import { bar } from "./legacy.js";
+console.log(foo, bar);
+`;
+    await writeFile(join(dir, "legacy.js"), "export const foo = 1, bar = 2;\n");
+    await writeFile(file, content);
+    const diagnostics = () =>
+      ts
+        .getPreEmitDiagnostics(
+          ts.createProgram([file], {
+            strict: true,
+            noEmit: true,
+            allowJs: false,
+            types: [],
+            module: ts.ModuleKind.ESNext,
+            moduleResolution: ts.ModuleResolutionKind.Bundler,
+          }),
+        )
+        .map((diagnostic) => ({
+          code: diagnostic.code,
+          message: ts.flattenDiagnosticMessageText(diagnostic.messageText, "\n"),
+        }));
+
+    assert.deepEqual(diagnostics(), []);
+    await writeFile(file, organizeImports(content, file));
+    assert.deepEqual(diagnostics(), []);
+  });
+
+  test("preserves an exported declaration between duplicate imports", () => {
+    const content = `import { foo } from "./value.ts";
+export const saved = foo;
+import { bar } from "./value.ts";
+console.log(saved, bar);
+`;
+
+    const result = organizeImports(content, "consumer.ts");
+
+    assert.ok(result.includes("export const saved = foo;"), "the interleaved export must survive");
+    assert.ok(result.includes("console.log(saved, bar);"));
+  });
+
+  test("merges adjacent imports from the same module", () => {
+    const result = organizeImports(
+      'import { foo } from "./value.ts";\nimport { bar } from "./value.ts";\nfoo(bar);\n',
+      "consumer.ts",
+    );
+
+    assert.equal((result.match(/from "\.\/value\.ts"/g) || []).length, 1);
+    assert.ok(result.includes('import { bar, foo } from "./value.ts";'));
+    assert.ok(result.endsWith("foo(bar);\n"));
+  });
+
+  test("leaves content without duplicate imports byte for byte unchanged", () => {
+    const content = `#!/usr/bin/env node\r\n"use client";\r\n// Header\r\nimport { foo } from './foo.ts';\r\nexport const saved = foo;\r\nimport './effect.ts';\r\n`;
+
+    assert.equal(organizeImports(content, "consumer.ts"), content);
+  });
+
+  test("keeps import groups separate across statements", () => {
+    const content = `import { foo } from "./value.ts";
+import { a } from "./other.ts";
+export const saved = foo;
+import "./effect.ts";
+function sum() { return a + b; }
+import { bar } from "./value.ts";
+console.log(saved, bar, sum());
+import { b } from "./other.ts";
+`;
+
+    const result = organizeImports(content, "consumer.ts");
+
+    assert.equal(result, content);
+  });
+
+  test("keeps variable declarations separate from array expressions across imports", () => {
+    const content = `import { foo } from "./value.ts";
+const saved = foo
+import { bar } from "./value.ts";
+[bar].forEach(console.log);
+`;
+    assert.equal(organizeImports(content, "consumer.ts"), content);
+  });
+
+  test("preserves type-only imports when coalescing returns multiple declarations", () => {
+    const content = `import { foo } from "./value.ts";
+import type { Value } from "./value.ts";
+const saved: Value = foo;
+`;
+    const result = organizeImports(content, "consumer.ts");
+    const parsed = ts.createSourceFile("consumer.ts", result, ts.ScriptTarget.Latest, true);
+    const imports = parsed.statements.filter(ts.isImportDeclaration);
+    assert.equal(imports.length, 2);
+    assert.deepEqual(
+      imports.map((imp) => imp.importClause?.isTypeOnly),
+      [false, true],
+    );
+    assert.ok(result.includes("const saved: Value = foo;"));
+  });
+
+  test("keeps ts-ignore directives attached to their imports", () => {
+    const content = `// @ts-ignore The dependency has no types.
+import { foo } from "./legacy.js";
+// @ts-ignore The dependency has no types.
+import { bar } from "./legacy.js";
+console.log(foo, bar);
+`;
+    assert.equal(organizeImports(content, "consumer.ts"), content);
+  });
+
+  for (const prefix of ["#!/usr/bin/env node\n", '#!/usr/bin/env node\n"use client";\n']) {
+    test(`preserves the prologue ${JSON.stringify(prefix)}`, () => {
+      const content = `${prefix}// License header
+import { foo } from "./value.ts";
+import { bar } from "./value.ts";
+console.log(foo, bar);
+`;
+
+      const result = organizeImports(content, "consumer.ts");
+
+      assert.ok(result.startsWith(`${prefix}// License header\n`));
+      assert.equal((result.match(/License header/g) || []).length, 1);
+      assert.equal((result.match(/from "\.\/value\.ts"/g) || []).length, 1);
+    });
+  }
+
+  test("preserves comments around imports and interleaved statements exactly once", () => {
+    const content = `// Header
+import { foo } from "./value.ts"; // First import
+// Public export
+export const saved = foo;
+/* Second import */
+import { /* Named binding */ bar } from "./value.ts"; // Last import
+// Consumer
+console.log(saved, bar);
+`;
+
+    const result = organizeImports(content, "consumer.ts");
+
+    for (const comment of [
+      "// Header",
+      "// First import",
+      "// Public export",
+      "/* Second import */",
+      "/* Named binding */",
+      "// Last import",
+      "// Consumer",
+    ]) {
+      assert.equal(result.split(comment).length - 1, 1, comment);
+    }
+    assert.ok(result.includes("// Public export\nexport const saved = foo;"));
+    assert.ok(result.includes("// Consumer\nconsole.log(saved, bar);"));
+  });
+});