refactor: move out skip resolution logic from package requester#10439
Conversation
3c2d4de to
4877d0e
Compare
| const skipResolution = resolution && !options.update | ||
| let forceFetch = false | ||
| let updated = false | ||
| let resolvedVia: string | undefined | ||
| let publishedAt: string | undefined |
There was a problem hiding this comment.
Thanks for the cleanup here. The let reassignments in the old code path made it hard to factor out functions that weren't nested. I was planning a similar refactor — I'm glad we ended up thinking the same thing!
There was a problem hiding this comment.
Pull request overview
This PR refactors the package resolution optimization logic by moving it from the package-requester into the individual resolvers. The key motivation is to simplify the package-requester and allow resolvers to handle their own caching/skip strategies.
Changes:
- Moved manifest peeking optimization from package-requester to npm-resolver, which now checks the store directly when storeDir is provided
- Added skip-resolution logic to git-resolver and local-resolver to reuse existing resolutions when not updating
- Simplified package-requester by always calling resolve and passing currentPkg information through
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg-manager/package-requester/src/packageRequester.ts | Removed peekPackageFromStore logic, now always calls resolve with currentPkg, simplified conditional resolution flow |
| resolving/npm-resolver/src/index.ts | Added peekManifestFromStore to check store before fetching metadata, implements fast path for cached packages |
| resolving/local-resolver/src/index.ts | Added skip logic to reuse existing resolutions for local files/directories when integrity/path unchanged |
| resolving/git-resolver/src/index.ts | Added skip logic to reuse existing git/tarball resolutions when not updating |
| resolving/resolver-base/src/index.ts | Added currentPkg to ResolveOptions interface to pass current resolution info to resolvers |
| resolving/npm-resolver/package.json | Added @pnpm/store.cafs dependency and @pnpm/worker peer dependency |
| resolving/npm-resolver/tsconfig.json | Added references to store/cafs and worker packages |
| testing/temp-store/src/index.ts | Fixed storeDir initialization order to pass to createClient |
| pnpm/test/hooks.ts | Added comment explaining removal of optional dependencies from dependencies field |
| Various test files | Updated all tests to pass storeDir parameter to createResolveFromNpm |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Fast path: if we have a current resolution with integrity, try to peek the manifest from the store. | ||
| // This avoids the expensive metadata fetch from the registry. | ||
| // We do this AFTER ensuring the spec is valid for this resolver to avoids hijacking other resolvers. | ||
| if (ctx.peekManifestFromStore && opts.currentPkg?.resolution && !opts.update) { | ||
| const currentResolution = opts.currentPkg.resolution | ||
| // Only use this optimization for tarball resolutions with integrity (npm packages) | ||
| if ('tarball' in currentResolution && currentResolution.integrity) { | ||
| const manifest = await ctx.peekManifestFromStore({ | ||
| id: opts.currentPkg.id, | ||
| integrity: currentResolution.integrity, | ||
| name: opts.currentPkg.name, | ||
| version: opts.currentPkg.version, | ||
| }) | ||
| // Verify the manifest matches what we expect | ||
| if (manifest?.name && manifest?.version) { | ||
| const id = `${manifest.name}@${manifest.version}` as PkgResolutionId | ||
| // Only return if the ID matches what we have in currentPkg | ||
| if (id === opts.currentPkg.id) { | ||
| return { | ||
| id, | ||
| latest: manifest.version, // Best we can do without fetching metadata | ||
| manifest, | ||
| resolution: currentResolution as TarballResolution, | ||
| resolvedVia: 'npm-registry', | ||
| publishedAt: undefined, // Don't have this without metadata | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The fast path optimization doesn't verify that the cached package satisfies the requested spec. When a bareSpecifier is updated (e.g., from "^1.0.0" to "^2.0.0" in package.json), but update is false, the fast path might return a cached package that doesn't satisfy the new spec. The spec is parsed on line 310-312 but never checked against the manifest returned from the store. Consider adding validation that the manifest version satisfies the spec range before returning the cached result, or document that this fast path is only valid when bareSpecifier hasn't changed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Also remove optional deps from dependencies since npm duplicates them there | ||
| for (const dep of Object.keys(pkg.optionalDependencies)) { | ||
| delete pkg.dependencies?.[dep] | ||
| } |
There was a problem hiding this comment.
The pnpmfile modification that removes optional dependencies from the dependencies field appears to be an unrelated change. While the added comment explains it handles npm's behavior of duplicating optional dependencies in the dependencies field, this change doesn't seem connected to the PR's stated purpose of "moving out skip resolution logic from package requester". Consider separating unrelated changes into their own PR for clearer change tracking and easier review.
|
|
||
| export interface ResolverFactoryOptions { | ||
| cacheDir: string | ||
| storeDir?: string |
There was a problem hiding this comment.
I tried to make it a required field but unfortunately we use the npm-resolver in multiple places where we currently don't have access to the storeDir.
|
These changes will also fix some edge cases with blockExoticSubdeps. The changes are big in a sensitive area. We will only ship them in v11. |
The forceFetch field was added to the internal ResolveResult in pnpm#10439 but wasn't exposed to custom resolvers. This allows custom resolvers to force re-fetching a package even if it exists in the store.
No description provided.