From f46ffdafc1226490a0496700888543cf2e1ca59a Mon Sep 17 00:00:00 2001 From: Lucas Braum Date: Sun, 18 Oct 2026 10:55:50 -0700 Subject: [PATCH] fix: keep an undeclared transitive the repo imports through the ERESOLVE retry (UPT-109) When npm's ERESOLVE retry re-resolves the lockfile and a package the repo imports without declaring it leaves the top level of node_modules, run the same retry step again keeping that package and every locked package that requires it. If that ERESOLVEs, the first retry stands. Either way the report lists it as "Imported but not declared" and suggests adding it to package.json; package.json is never changed. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Lucas Braum --- .changeset/keep-undeclared-imports.md | 5 + packages/cli/src/format-fix.test.ts | 23 ++++ packages/cli/src/format-fix.ts | 6 + .../core/src/fix/managers/eresolve.test.ts | 82 +++++++++++-- packages/core/src/fix/managers/upgrade.ts | 109 ++++++++++++++++-- packages/core/src/fix/report.test.ts | 19 +++ packages/core/src/fix/report.ts | 9 +- packages/core/src/index.ts | 6 +- 8 files changed, 242 insertions(+), 17 deletions(-) create mode 100644 .changeset/keep-undeclared-imports.md diff --git a/.changeset/keep-undeclared-imports.md b/.changeset/keep-undeclared-imports.md new file mode 100644 index 0000000..73c3a8c --- /dev/null +++ b/.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. diff --git a/packages/cli/src/format-fix.test.ts b/packages/cli/src/format-fix.test.ts index 2cd319b..2bda4c7 100644 --- a/packages/cli/src/format-fix.test.ts +++ b/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 = { diff --git a/packages/cli/src/format-fix.ts b/packages/cli/src/format-fix.ts index 614ddab..be5f7a3 100644 --- a/packages/cli/src/format-fix.ts +++ b/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) diff --git a/packages/core/src/fix/managers/eresolve.test.ts b/packages/core/src/fix/managers/eresolve.test.ts index 6b4d563..baf587b 100644 --- a/packages/core/src/fix/managers/eresolve.test.ts +++ b/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 }; +type Entry = { + version: string; + dependencies?: Record; + peerDependencies?: Record; +}; type Lock = { packages: Record }; const registry: Record = { ui: [{ version: '1.0.0' }, { version: '2.0.0' }], @@ -27,6 +31,16 @@ const registry: Record = { { 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, entries: Record) => ({ name: 'app', @@ -46,13 +60,16 @@ afterEach(() => { rmSync(root, { recursive: true, force: true }); }); -function repo(conflict: (lock: Lock) => boolean) { +function repo(conflict: (lock: Lock) => boolean, declared: Record = ranges) { root = mkdtempSync(join(tmpdir(), 'uptide-eresolve-')); const manifest = (deps: Record) => 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', '.'], @@ -60,7 +77,7 @@ function repo(conflict: (lock: Lock) => boolean) { ]) 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,8 +88,11 @@ function repo(conflict: (lock: Lock) => boolean) { const deps: Record = 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 = {}; - 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 = @@ -80,6 +100,7 @@ function repo(conflict: (lock: Lock) => boolean) { ? 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); +}); diff --git a/packages/core/src/fix/managers/upgrade.ts b/packages/core/src/fix/managers/upgrade.ts index b559a72..7096244 100644 --- a/packages/core/src/fix/managers/upgrade.ts +++ b/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 { +function dropEntries(text: string, names?: Set, keep = new Set()): 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 { const names = new Set(moved); @@ -98,12 +114,59 @@ function peerHolders(text: string, moved: string[]): Set { 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 { + const found = new Set(names); + const entries = Object.entries | 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 { + 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(); + 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, @@ -111,7 +174,7 @@ async function resolving( beforeText: string, moved: string[], attempt: () => Promise, -): 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 { + 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 { + const report = fixture('storefront-zod'); + report.lockfile = { + manager: 'npm', + file: 'package-lock.json', + added: [], + removed: [], + changed: [], + allowed: [], + undeclared: [{ name: 'picker-types', version: '1.0.0', kept: true }], + }; + for (const body of [prBody(report), migrationBody(report)]) { + expect(body).toContain('### Imported but not declared'); + expect(body).toContain( + '- 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.', + ); + } +}); + it('renders scoped declarative behavior decisions in the PR body', async () => { const { nextPack } = await import('../packs/next/index.js'); const { packDecisions } = await import('../packs/decisions.js'); diff --git a/packages/core/src/fix/report.ts b/packages/core/src/fix/report.ts index 36831d7..27317ea 100644 --- a/packages/core/src/fix/report.ts +++ b/packages/core/src/fix/report.ts @@ -13,7 +13,7 @@ import { describeTransitive, type OutOfScopeChange, } from './managers/lock-guard.js'; -import { describeEresolveRetry } from './managers/upgrade.js'; +import { describeEresolveRetry, describeUndeclared } from './managers/upgrade.js'; import { noPackBlockers } from './no-pack.js'; import { uniqueSites } from './paired.js'; import { groupPeerBlockers, peerDescription } from './peer-preflight.js'; @@ -1035,6 +1035,13 @@ export function renderMigration( '', `${describeEresolveRetry(report.lockfile.eresolveRetry)}.`, ); + if (report.lockfile?.undeclared?.length) + lines.push( + '', + '### Imported but not declared', + '', + ...report.lockfile.undeclared.map((u) => `- ${cell(describeUndeclared(u))}`), + ); if (report.lockfile?.peerReresolved?.length) { lines.push( '', diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index fb879c6..3aa4dac 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -136,7 +136,11 @@ export { type OutOfScopeChange, type TransitiveChange, } from './fix/managers/lock-guard.js'; -export { describeEresolveRetry, InstallBlockedError } from './fix/managers/upgrade.js'; +export { + describeEresolveRetry, + describeUndeclared, + InstallBlockedError, +} from './fix/managers/upgrade.js'; export { groupPeerBlockers, type PeerBlockerGroup,