Skip to content

fix: onlyBuiltDependencies is an empty array#22

Open
btea wants to merge 1 commit into
mainfrom
fix/createAllowBuildFunction-policy-update
Open

fix: onlyBuiltDependencies is an empty array#22
btea wants to merge 1 commit into
mainfrom
fix/createAllowBuildFunction-policy-update

Conversation

@btea

@btea btea commented Feb 19, 2025

Copy link
Copy Markdown
Member

@btea btea requested a review from zkochan February 19, 2025 04:38
@btea

btea commented Feb 22, 2025

Copy link
Copy Markdown
Member Author

The repository here has not been updated for a long time and there are problems with ci. Do we should continue updating here?

Comment thread builder/policy/policy.ts
}
): undefined | ((pkgName: string) => boolean) {
if (opts.onlyBuiltDependenciesFile || opts.onlyBuiltDependencies != null) {
if (opts.onlyBuiltDependenciesFile || opts.onlyBuiltDependencies?.length) {

@zkochan zkochan Feb 25, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct. If the array is empty it means that no dependencies are allowed to be built.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zkochan zkochan Feb 25, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all builds are ignored, then pnpm rebuild should build nothing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, pnpm rebuild will add all dependencies that do not need to execute lifecycle scripts to ignoredBuilds and write them to the .modules.yaml file, causing pnpm approve-builds to list these dependencies.

@secondl1ght

Copy link
Copy Markdown

I didn't review this PR to see if it fixes the issue - but just wanted to post my comment here that I was also able to reproduce the issue that this is trying to solve: pnpm/pnpm#9129 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants