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,