Conversation
|
I don't think that this is the right solution. You are making it impossible to set this setting globally. Users should be able to allow all scripts in all projects on the computer by globally setting dangerouslyAllowAllBuilds to true. But if a project has an allow list or a disallow list of built dependencies, then prefer those local settings. |
There was a problem hiding this comment.
Pull Request Overview
Fix conflicts between global and local settings for running lifecycle scripts by extracting build-related config keys and applying them with correct precedence.
- Introduces
depsBuildConfig.tshelpers to remove and reapply build settings - Refactors
getConfigto strip build flags from the base config and conditionally merge global or local values - Adds end-to-end tests in
global.tscovering bothdangerously-allow-all-buildsandonlyBuiltDependenciesbehaviors
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pnpm/test/install/global.ts | Adds tests to verify global versus local build flag precedence |
| config/config/src/index.ts | Refactors getConfig to separate and conditionally merge build keys |
| config/config/src/depsBuildConfig.ts | New utilities for extracting/removing build-related settings |
| .changeset/cuddly-plants-sleep.md | Documents version bump and PR purpose |
Comments suppressed due to low confidence (3)
config/config/src/index.ts:230
- [nitpick] The variable name
globalConfigWithoutDepsBuildSettingsis quite long and may reduce readability. Consider renaming it to something shorter likebaseGlobalConfigorglobalBaseConfig.
const globalConfigWithoutDepsBuildSettings: ConfigWithDeprecatedSettings = Object.fromEntries(
config/config/src/index.ts:382
- [nitpick] Add a brief comment explaining why build settings are removed and reapplied in the global branch to clarify the intended precedence logic for future maintainers.
if (opts.cliOptions['global']) {
pnpm/test/install/global.ts:95
- [nitpick] You might group or annotate platform-specific tests together (e.g., using
test.skipwith a message) so it's clear why they are no-ops on non-Linux environments.
if (process.platform !== 'linux') return
config/config/src/index.ts
Outdated
| import which from 'which' | ||
| import { inheritAuthConfig } from './auth' | ||
| import { checkGlobalBinDir } from './checkGlobalBinDir' | ||
| import { hasDepsBuild, removeDepsBuild } from './depsBuildConfig' |
There was a problem hiding this comment.
The names of these functions are not descriptive enough.
| import { hasDepsBuild, removeDepsBuild } from './depsBuildConfig' | |
| import { hasDependencyBuildOptions | |
| , extractAndRemoveDependencyBuildOptions } from './depsBuildConfig' |
config/config/src/index.ts
Outdated
| const globalConfigWithoutDepsBuildSettings: ConfigWithDeprecatedSettings = Object.fromEntries( | ||
| rcOptions.map((configKey) => [camelcase(configKey, { locale: 'en-US' }), npmConfig.get(configKey)]) | ||
| ) as ConfigWithDeprecatedSettings | ||
| const globalDepsBuildConfig = removeDepsBuild(globalConfigWithoutDepsBuildSettings) | ||
|
|
||
| const pnpmConfig: ConfigWithDeprecatedSettings = Object.assign(globalConfigWithoutDepsBuildSettings, configFromCliOpts) as unknown as ConfigWithDeprecatedSettings |
There was a problem hiding this comment.
isn't it easier just to skip the removal in case of global install?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
It's not easier. This method would require every code path below to refrain from inheriting the settings from local config or CLI args or etc, increasing complexity. It is simplier to just inherit them all then remove them later.
config/config/src/index.ts
Outdated
| rcOptions.map((configKey) => [camelcase(configKey, { locale: 'en-US' }), npmConfig.get(configKey)]) as any, // eslint-disable-line | ||
| ), configFromCliOpts) as unknown as ConfigWithDeprecatedSettings | ||
|
|
||
| const globalConfigWithoutDepsBuildSettings: ConfigWithDeprecatedSettings = Object.fromEntries( |
There was a problem hiding this comment.
the name of this variable is confusing because at this point it has the build settings.
Fixes #9628