feat: support allowing the build of specific versions of dependencies #10104
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for specifying exact versions of dependencies when controlling which packages are allowed to build, enhancing the granularity of build policy configuration.
Key changes:
- Updated the
AllowBuildtype signature to accept both package name and version parameters - Introduced version-aware filtering in the
@pnpm/builder.policypackage with support for exact version matching - Modified all callsites to pass version information to
allowBuildfunctions
Reviewed Changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/types/src/config.ts | Added new AllowBuild type definition accepting name and version |
| builder/policy/src/index.ts | New package implementing version-aware build policy logic |
| config/matcher/src/index.ts | Added expandPackageVersionSpecs function to handle version-specific package specs |
| pkg-manager/headless/src/index.ts | Updated allowBuild calls to include version parameter |
| pkg-manager/core/src/install/link.ts | Updated allowBuild type annotations and invocations |
| exec/build-modules/src/index.ts | Updated to pass version to allowBuild function |
| exec/plugin-commands-rebuild/src/implementation/index.ts | Updated rebuild logic to use version-aware build policy |
| deps/graph-builder/src/lockfileToDepGraph.ts | Added version field to DependenciesGraphNode |
| pnpm-workspace.yaml | Added builder packages to workspace and removed catalog dependency |
| pkg-manager/core/test/install/lifecycleScripts.ts | Added test coverage for exact version matching in build policies |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (opts.onlyBuiltDependenciesFile != null || opts.onlyBuiltDependencies != null) { | ||
| const onlyBuiltDeps = opts.onlyBuiltDependencies ?? [] | ||
| if (opts.onlyBuiltDependenciesFile) { | ||
| onlyBuiltDeps.push(...JSON.parse(fs.readFileSync(opts.onlyBuiltDependenciesFile, 'utf8'))) |
There was a problem hiding this comment.
Missing error handling for file read and JSON parse operations. If the file doesn't exist or contains invalid JSON, this will throw an unhelpful error. Add try-catch with a clear error message indicating the file path and nature of the problem.
| onlyBuiltDeps.push(...JSON.parse(fs.readFileSync(opts.onlyBuiltDependenciesFile, 'utf8'))) | |
| try { | |
| const fileContent = fs.readFileSync(opts.onlyBuiltDependenciesFile, 'utf8'); | |
| onlyBuiltDeps.push(...JSON.parse(fileContent)); | |
| } catch (err) { | |
| throw new Error( | |
| `Failed to read or parse JSON from file "${opts.onlyBuiltDependenciesFile}": ${err instanceof Error ? err.message : String(err)}` | |
| ); | |
| } |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 45 out of 46 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| function parseExactVersionsUnion (versionsStr: string): string[] | null { | ||
| const versions: string[] = [] | ||
| for (const versionRaw of versionsStr.split('||')) { | ||
| const version = semver.valid(versionRaw) |
There was a problem hiding this comment.
The version string is not being trimmed before validation. Whitespace in version unions like '1.0.0||1.0.1 || 1.0.2' will cause semver.valid() to return null. Add .trim() before calling semver.valid().
| const version = semver.valid(versionRaw) | |
| const version = semver.valid(versionRaw.trim()) |
|
|
||
| type Matcher = (input: string) => boolean | ||
| type MatcherWithIndex = (input: string) => number | ||
| export type Matcher = (input: string) => boolean |
There was a problem hiding this comment.
These types were previously not exported but are now exported. However, they're being imported in config/version-policy/src/index.ts which creates a dependency on @pnpm/matcher. Consider documenting this breaking change or verifying that all consumers are updated to import from the correct location.
| export type Matcher = (input: string) => boolean | |
| export type Matcher = (input: string) => boolean | |
| // BREAKING CHANGE: MatcherWithIndex is now exported from @pnpm/matcher. Consumers should import from this location. |
close #10076