fix: keep an undeclared transitive the repo imports through the ERESOLVE retry (UPT-109)

.changeset/keep-undeclared-imports.md

@@ -0,0 +1,5 @@
+---
+"uptide": patch
+---
+
+`fix` on npm no longer loses a package the repo imports without declaring it when the ERESOLVE retry re-resolves the lockfile. If the retry drops such a package from the top of `node_modules`, it runs again keeping that package and the locked packages that require it at their old versions; if that conflicts, the first retry stands. Either way the summary and the PR body name the package under "Imported but not declared" and suggest adding it to package.json. `fix` never adds it for you.

packages/cli/src/format-fix.test.ts

@@ -171,6 +171,29 @@ it('says which ERESOLVE retry resolved the lockfile', () => {
   );
 });
 
+it('names an imported undeclared package the re-resolve kept or dropped (UPT-109)', () => {
+  const report = fixReport(true);
+  report.lockfile = {
+    manager: 'npm',
+    file: 'package-lock.json',
+    added: [],
+    removed: [],
+    changed: [],
+    allowed: [],
+    undeclared: [
+      { name: 'picker-types', version: '1.0.0', kept: true },
+      { name: 'old-types', version: '2.1.0', kept: false },
+    ],
+  };
+  const out = formatFixSummary(report, { color: false });
+  expect(out).toContain(
+    'Imported but not declared\n  picker-types 1.0.0 is imported by the repo but not declared in package.json. The re-resolve would have dropped it; it was kept at 1.0.0. Add it to package.json.\n',
+  );
+  expect(out).toContain(
+    'old-types 2.1.0 is imported by the repo but not declared in package.json. The new lockfile no longer has it, so those imports fail. Add it to package.json.',
+  );
+});
+
 it('lists transitive changes under the package that moved them, two at a time', () => {
   const report = fixReport(true);
   report.lockfile = {

packages/cli/src/format-fix.ts

@@ -7,6 +7,7 @@ import {
   describeOutOfScope,
   describePhases,
   describeTransitive,
+  describeUndeclared,
   type FixReport,
   fixVerdict,
   groupPeerBlockers,
@@ -129,6 +130,11 @@ export function formatFixSummary(report: FixReport, opts: FormatFixOptions = {})
       '',
     );
   }
+  if (report.lockfile?.undeclared?.length) {
+    lines.push(colors.bold('Imported but not declared'));
+    for (const u of report.lockfile.undeclared) lines.push(`  ${describeUndeclared(u)}`);
+    lines.push('');
+  }
   if (report.lockfile?.peerReresolved?.length) {
     lines.push(colors.bold('Re-resolved because a peer changed'));
     for (const p of report.lockfile.peerReresolved)

packages/core/src/fix/managers/eresolve.test.ts

@@ -1,5 +1,5 @@
 import { execFileSync } from 'node:child_process';
-import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
+import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { join, resolve } from 'node:path';
 import { afterEach, expect, it, vi } from 'vitest';
@@ -14,7 +14,11 @@ const eresolve = readFileSync(
   resolve(import.meta.dirname, '../../../../../fixtures/lockfiles/eresolve/npm-11.txt'),
   'utf8',
 );
-type Entry = { version: string; peerDependencies?: Record<string, string> };
+type Entry = {
+  version: string;
+  dependencies?: Record<string, string>;
+  peerDependencies?: Record<string, string>;
+};
 type Lock = { packages: Record<string, Entry> };
 const registry: Record<string, Entry[]> = {
   ui: [{ version: '1.0.0' }, { version: '2.0.0' }],
@@ -27,6 +31,16 @@ const registry: Record<string, Entry[]> = {
     { version: '1.1.0', peerDependencies: { ui: '^1.0.0 || ^2.0.0' } },
   ],
   unrelated: [{ version: '1.0.0' }, { version: '1.2.0' }],
+  // UPT-109: picker 1.1.0 no longer depends on picker-types, which the repo imports undeclared.
+  picker: [
+    {
+      version: '1.0.0',
+      dependencies: { 'picker-types': '^1.0.0' },
+      peerDependencies: { ui: '^1.0.0 || ^2.0.0' },
+    },
+    { version: '1.1.0', peerDependencies: { ui: '^1.0.0 || ^2.0.0' } },
+  ],
+  'picker-types': [{ version: '1.0.0' }],
 };
 const lockOf = (deps: Record<string, string>, entries: Record<string, Entry>) => ({
   name: 'app',
@@ -46,21 +60,24 @@ afterEach(() => {
   rmSync(root, { recursive: true, force: true });
 });
 
-function repo(conflict: (lock: Lock) => boolean) {
+function repo(conflict: (lock: Lock) => boolean, declared: Record<string, string> = ranges) {
   root = mkdtempSync(join(tmpdir(), 'uptide-eresolve-'));
   const manifest = (deps: Record<string, string>) =>
     writeFileSync(join(root, 'package.json'), JSON.stringify({ name: 'app', dependencies: deps }));
-  manifest(ranges);
-  const before = Object.fromEntries(Object.keys(ranges).map((n) => [n, first(n)]));
-  writeFileSync(join(root, 'package-lock.json'), JSON.stringify(lockOf(ranges, before)));
+  manifest(declared);
+  const before = Object.fromEntries(Object.keys(declared).map((n) => [n, first(n)]));
+  if (declared.picker) before['picker-types'] = first('picker-types');
+  writeFileSync(join(root, 'package-lock.json'), JSON.stringify(lockOf(declared, before)));
+  mkdirSync(join(root, 'src'));
+  writeFileSync(join(root, 'src/date.ts'), "import type { Range } from 'picker-types/range';\n");
   for (const args of [
     ['init', '-q'],
     ['add', '.'],
     ['-c', 'user.name=t', '-c', 'user.email=t@t', 'commit', '-qm', 'parent'],
   ])
     execFileSync('git', args, { cwd: root });
   // What the plan writes: the lead and its companion bumped.
-  manifest({ ...ranges, ui: '^2.0.0', 'ui-dom': '^2.0.0' });
+  manifest({ ...declared, ui: '^2.0.0', 'ui-dom': '^2.0.0' });
   const installs: { args: string[]; lock: Lock }[] = [];
   vi.spyOn(process, 'command').mockImplementation(async (_root, _bin, args) => {
     if (args[0] === '--version') return { code: 0, output: '11.6.2', timeout: false };
@@ -71,15 +88,19 @@ function repo(conflict: (lock: Lock) => boolean) {
     const deps: Record<string, string> = JSON.parse(
       readFileSync(join(root, 'package.json'), 'utf8'),
     ).dependencies;
+    // Hoisted and flat: every dependency at the top level; unrequired entries are pruned.
     const entries: Record<string, Entry> = {};
-    for (const [name, range] of Object.entries(deps)) {
+    const wanted = Object.entries(deps);
+    for (const [name, range] of wanted) {
+      if (entries[name]) continue;
       const locked = lock.packages[`node_modules/${name}`];
       const versions = (registry[name] ?? []).map((e) => e.version);
       const version =
         locked && satisfies(locked.version, range)
           ? locked.version
           : (maxSatisfying(versions, range) as string);
       entries[name] = registry[name]?.find((e) => e.version === version) as Entry;
+      wanted.push(...Object.entries(entries[name].dependencies ?? {}));
     }
     writeFileSync(join(root, 'package-lock.json'), JSON.stringify(lockOf(deps, entries)));
     return { code: 0, output: '', timeout: false };
@@ -160,3 +181,48 @@ it('ends blocked, naming the packages npm reported, when every step fails', asyn
   expect(installs).toHaveLength(3);
   neverForced(installs);
 });
+
+// UPT-109: picker re-resolves to 1.1.0, which no longer brings picker-types; src/date.ts imports it.
+const withPicker = { ...ranges, picker: '^1.0.0' };
+
+it('keeps an undeclared package the repo imports when the re-resolve would drop it', async () => {
+  const installs = repo(
+    (lock) => lock.packages['node_modules/toggle']?.version === '1.0.0',
+    withPicker,
+  );
+  const report = await upgradeInstall(root, upgrade);
+  expect(report.eresolveRetry).toMatchObject({
+    step: 'moved entries',
+    dropped: ['toggle', 'ui', 'ui-dom'],
+  });
+  // The first retry dropped picker (it peers on ui), so npm pruned picker-types; the next one
+  // started with picker locked too, after the 1 + 3 installs of the failed and the first retry.
+  expect(locked(installs[1]?.lock)).toEqual(['unrelated', 'picker-types']);
+  expect(locked(installs[4]?.lock)).toEqual(['unrelated', 'picker', 'picker-types']);
+  const lock = JSON.parse(readFileSync(join(root, 'package-lock.json'), 'utf8'));
+  expect(lock.packages['node_modules/picker-types'].version).toBe('1.0.0');
+  expect(lock.packages['node_modules/picker'].version).toBe('1.0.0');
+  expect(report.undeclared).toEqual([{ name: 'picker-types', version: '1.0.0', kept: true }]);
+  // Never declared on the repo's behalf.
+  expect(
+    JSON.parse(readFileSync(join(root, 'package.json'), 'utf8')).dependencies,
+  ).not.toHaveProperty('picker-types');
+  neverForced(installs);
+});
+
+it('reports an imported undeclared package it cannot keep', async () => {
+  // picker 1.0.0 conflicts too: keeping it ERESOLVEs, so the retry without it stands.
+  const installs = repo(
+    (lock) =>
+      lock.packages['node_modules/toggle']?.version === '1.0.0' ||
+      lock.packages['node_modules/picker']?.version === '1.0.0',
+    withPicker,
+  );
+  const report = await upgradeInstall(root, upgrade);
+  expect(report.eresolveRetry?.step).toBe('moved entries');
+  const lock = JSON.parse(readFileSync(join(root, 'package-lock.json'), 'utf8'));
+  expect(lock.packages['node_modules/picker'].version).toBe('1.1.0');
+  expect(lock.packages['node_modules/picker-types']).toBeUndefined();
+  expect(report.undeclared).toEqual([{ name: 'picker-types', version: '1.0.0', kept: false }]);
+  neverForced(installs);
+});

packages/core/src/fix/managers/upgrade.ts

@@ -1,5 +1,5 @@
 import { existsSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
-import { join } from 'node:path';
+import { dirname, join } from 'node:path';
 import { parseNpm, parsePnpm, parseYarn } from '../../adapters/typescript/lockfile.js';
 import { UptideError } from '../../errors.js';
 import { satisfies } from '../../fetch/range.js';
@@ -40,7 +40,17 @@ export interface InstallReport extends LockDiff {
   file: string;
   /** npm's lock-only install ERESOLVEd from the old lockfile; the smaller retry that resolved (UPT-107). */
   eresolveRetry?: EresolveRetry;
+  /** Packages the repo imports without declaring them that the new lockfile would drop (UPT-109). */
+  undeclared?: Undeclared[];
 }
+export interface Undeclared {
+  name: string;
+  /** The version the new lockfile keeps, or the one the old lockfile had when it is gone. */
+  version: string;
+  kept: boolean;
+}
+export const describeUndeclared = (u: Undeclared): string =>
+  `${u.name} ${u.version} is imported by the repo but not declared in package.json. ${u.kept ? `The re-resolve would have dropped it; it was kept at ${u.version}.` : 'The new lockfile no longer has it, so those imports fail.'} Add it to package.json.`;
 type RetryStep = 'moved entries' | 'whole lockfile';
 export interface EresolveRetry {
   step: RetryStep;
@@ -81,15 +91,21 @@ const isEresolve = (error: unknown) =>
  * The old lockfile without the entries of `names` and everything installed below them, or
  * without every installed entry (undefined): npm resolves those afresh. Links stay.
  */
-function dropEntries(text: string, names?: Set<string>): string {
+function dropEntries(text: string, names?: Set<string>, keep = new Set<string>()): string {
   const json = JSON.parse(text);
   for (const [key, entry] of Object.entries<{ link?: boolean }>(json.packages ?? {})) {
     const installed = key.split('node_modules/').slice(1);
     if (!installed.length || entry.link) continue;
-    if (!names || installed.some((n) => names.has(n.replace(/\/$/, '')))) delete json.packages[key];
+    const dropped = (n: string) => {
+      const name = n.replace(/\/$/, '');
+      return !keep.has(name) && (!names || names.has(name));
+    };
+    if (installed.some(dropped)) delete json.packages[key];
   }
   return `${JSON.stringify(json, null, 2)}\n`;
 }
+const installedName = (key: string) =>
+  key.slice(key.lastIndexOf('node_modules/') + 'node_modules/'.length);
 /** The moved packages and every locked package that declares one of them as a peer. */
 function peerHolders(text: string, moved: string[]): Set<string> {
   const names = new Set(moved);
@@ -98,20 +114,67 @@ function peerHolders(text: string, moved: string[]): Set<string> {
     packages,
   ))
     if (key.includes('node_modules/') && moved.some((m) => entry.peerDependencies?.[m]))
-      names.add(key.slice(key.lastIndexOf('node_modules/') + 'node_modules/'.length));
+      names.add(installedName(key));
   return names;
 }
+/** `names` and every locked package that depends on one of them, transitively. */
+function requirers(text: string, names: string[]): Set<string> {
+  const found = new Set(names);
+  const entries = Object.entries<Record<string, Record<string, string> | undefined>>(
+    JSON.parse(text).packages ?? {},
+  ).filter(([key]) => key.includes('node_modules/'));
+  for (let size = 0; size !== found.size; ) {
+    size = found.size;
+    for (const [key, entry] of entries)
+      if (
+        [entry.dependencies, entry.optionalDependencies, entry.peerDependencies].some((edges) =>
+          [...found].some((n) => edges?.[n]),
+        )
+      )
+        found.add(installedName(key));
+  }
+  return found;
+}
+/** What a bare import reaches in an npm lockfile: the root's and each workspace's top level. */
+function topLevel(text: string): Map<string, string> {
+  return new Map(
+    Object.entries<{ version?: string }>(JSON.parse(text).packages ?? {}).flatMap(([key, e]) => {
+      const installed = key.split('node_modules/').slice(1);
+      return installed.length === 1 && e.version ? [[installed[0] as string, e.version]] : [];
+    }),
+  );
+}
+const IMPORT =
+  /\b(?:from|import|require)\s*\(?\s*['"]((?:@[\w.-]+\/)?[\w-][\w.-]*)(?:\/[^'"]*)?['"]/g;
+/** Packages `afterText` no longer has at the top level that tracked source imports by name. */
+function droppedImports(root: string, beforeText: string, afterText: string): string[] {
+  const after = topLevel(afterText);
+  const gone = new Set([...topLevel(beforeText).keys()].filter((name) => !after.has(name)));
+  const imported = new Set<string>();
+  if (gone.size)
+    for (const file of git(root, 'ls-files', '-z').split('\0'))
+      if (
+        /\.[cm]?[jt]sx?$/.test(file) &&
+        !file.split('/').includes('node_modules') &&
+        existsSync(join(root, file))
+      )
+        for (const [, name] of readFileSync(join(root, file), 'utf8').matchAll(IMPORT))
+          if (gone.has(name as string)) imported.add(name as string);
+  return [...imported].sort();
+}
 /**
  * `attempt` from the old lockfile, then (npm ERESOLVE only) from smaller ones, smallest
  * re-resolve first (UPT-107). Returns the lockfile the successful attempt started from.
+ * A retry that drops a package the repo imports undeclared runs again keeping that package and
+ * every locked package that requires it; if that ERESOLVEs, the first retry stands (UPT-109).
  */
 async function resolving(
   pm: PackageManager,
   lockfile: string,
   beforeText: string,
   moved: string[],
   attempt: () => Promise<void>,
-): Promise<{ start: string; retry?: EresolveRetry }> {
+): Promise<{ start: string; retry?: EresolveRetry; imports?: string[] }> {
   let failure: unknown;
   try {
     await attempt();
@@ -131,10 +194,27 @@ async function resolving(
     writeFileSync(lockfile, start);
     try {
       await attempt();
-      return { start, retry: { step, dropped: names ? [...names].sort() : [], conflicts } };
     } catch (error) {
       if (!isEresolve(error)) throw error;
       failure = error;
+      continue;
+    }
+    const retry = { step, dropped: names ? [...names].sort() : [], conflicts };
+    const resolved = readFileSync(lockfile, 'utf8');
+    const imports = droppedImports(dirname(lockfile), beforeText, resolved);
+    if (!imports.length) return { start, retry };
+    const keep = requirers(beforeText, imports);
+    for (const name of moved) keep.delete(name);
+    const kept = dropEntries(beforeText, names, keep);
+    writeFileSync(lockfile, kept);
+    try {
+      await attempt();
+      const dropped = retry.dropped.filter((name) => !keep.has(name));
+      return { start: kept, retry: { ...retry, dropped }, imports };
+    } catch (error) {
+      if (!isEresolve(error)) throw error;
+      writeFileSync(lockfile, resolved);
+      return { start, retry, imports };
     }
   }
   throw new InstallBlockedError({
@@ -296,7 +376,7 @@ export async function upgradeInstall(root: string, upgrade: Upgrade): Promise<In
       return [file, readFileSync(join(root, file), 'utf8')];
     }),
   );
-  const { start, retry } = await resolving(pm, lockfile, beforeText, names, async () => {
+  const { start, retry, imports } = await resolving(pm, lockfile, beforeText, names, async () => {
     await installPinned(root, pm, upgrade.workspaces, names, (_, name) => version.get(name));
     if (pm.kind === 'yarn-berry') {
       // Restoring a broad range drops Yarn's temporary resolution; pin its descriptor in the
@@ -405,10 +485,25 @@ export async function upgradeInstall(root: string, upgrade: Upgrade): Promise<In
       'LOCKFILE_OUT_OF_SCOPE',
       'Verification install changed the validated lockfile',
     );
+  // Never declared on the repo's behalf: kept in the lockfile where it can be, reported always.
+  const undeclared: Undeclared[] = [];
+  if (pm.kind === 'npm') {
+    const [was, now] = [topLevel(beforeText), topLevel(afterText)];
+    const lost = droppedImports(root, beforeText, afterText);
+    for (const name of [...new Set([...(imports ?? []), ...lost])].sort()) {
+      const version = now.get(name);
+      undeclared.push(
+        version
+          ? { name, version, kept: true }
+          : { name, version: was.get(name) as string, kept: false },
+      );
+    }
+  }
   return {
     ...diff,
     manager: pm.kind,
     file: pm.lockfile,
     ...(retry ? { eresolveRetry: retry } : {}),
+    ...(undeclared.length ? { undeclared } : {}),
   };
 }