fix: shamefullyHoist set via updateConfig in .pnpmfile.cjs (#10519)

* fix: `shamefullyHoist` set via `updateConfig` in `.pnpmfile.cjs`

* refactor: consolidate derived config processing to cli-utils

Move shamefullyHoist → publicHoistPattern conversion from
config/config to cli-utils/getConfig.ts as suggested in review.

* test(config): update tests for derived config processing move

* refactor: move applyDerivedConfig to cli-utils

* refactor: move applyDerivedConfig to cli-utils

* test: use unit test for hoist: false in cli-utils

* revert: not needed changes

close #10271

.changeset/shaky-emus-notice.md

@@ -0,0 +1,7 @@
+---
+"@pnpm/cli-utils": patch
+"@pnpm/config": patch
+"pnpm": patch
+---
+
+Fix `shamefullyHoist` set via `updateConfig` in `.pnpmfile.cjs` not being converted to `publicHoistPattern` [#10271](https://github.com/pnpm/pnpm/issues/10271).

cli/cli-utils/src/getConfig.ts

@@ -59,6 +59,7 @@ export async function getConfig (
       }
     }
   }
+  applyDerivedConfig(config)
 
   if (opts.excludeReporter) {
     delete config.reporter // This is a silly workaround because @pnpm/core expects a function as opts.reporter
@@ -84,3 +85,35 @@ function isPluginName (configDepName: string): boolean {
   if (configDepName[0] !== '@') return false
   return configDepName.startsWith('@pnpm/plugin-') || configDepName.includes('/pnpm-plugin-')
 }
+
+// Apply derived config settings (hoist, shamefullyHoist, symlink)
+function applyDerivedConfig (config: Config): void {
+  if (config.hoist === false) {
+    delete config.hoistPattern
+  }
+  switch (config.shamefullyHoist) {
+  case false:
+    delete config.publicHoistPattern
+    break
+  case true:
+    config.publicHoistPattern = ['*']
+    break
+  default:
+    if (
+      (config.publicHoistPattern == null) ||
+        (config.publicHoistPattern === '') ||
+        (
+          Array.isArray(config.publicHoistPattern) &&
+          config.publicHoistPattern.length === 1 &&
+          config.publicHoistPattern[0] === ''
+        )
+    ) {
+      delete config.publicHoistPattern
+    }
+    break
+  }
+  if (!config.symlink) {
+    delete config.hoistPattern
+    delete config.publicHoistPattern
+  }
+}

cli/cli-utils/test/getConfig.test.ts

@@ -27,3 +27,18 @@ test('console a warning when the .npmrc has an env variable that does not exist'
   // eslint-disable-next-line no-template-curly-in-string
   expect(console.warn).toHaveBeenCalledWith(expect.stringContaining('Failed to replace env in config: ${ENV_VAR_123}'))
 })
+
+test('hoist: false removes hoistPattern', async () => {
+  prepare()
+
+  const config = await getConfig({
+    hoist: false,
+  }, {
+    workspaceDir: '.',
+    excludeReporter: false,
+    rcOptionsTypes: {},
+  })
+
+  expect(config.hoist).toBe(false)
+  expect(config.hoistPattern).toBeUndefined()
+})

config/config/src/index.ts

@@ -458,35 +458,6 @@ export async function getConfig (opts: {
   if (!pnpmConfig.stateDir) {
     pnpmConfig.stateDir = getStateDir(process)
   }
-  // eslint-disable-next-line @typescript-eslint/no-unnecessary-boolean-literal-compare
-  if (pnpmConfig.hoist === false) {
-    delete pnpmConfig.hoistPattern
-  }
-  switch (pnpmConfig.shamefullyHoist) {
-  case false:
-    delete pnpmConfig.publicHoistPattern
-    break
-  case true:
-    pnpmConfig.publicHoistPattern = ['*']
-    break
-  default:
-    if (
-      (pnpmConfig.publicHoistPattern == null) ||
-        (pnpmConfig.publicHoistPattern === '') ||
-        (
-          Array.isArray(pnpmConfig.publicHoistPattern) &&
-          pnpmConfig.publicHoistPattern.length === 1 &&
-          pnpmConfig.publicHoistPattern[0] === ''
-        )
-    ) {
-      delete pnpmConfig.publicHoistPattern
-    }
-    break
-  }
-  if (!pnpmConfig.symlink) {
-    delete pnpmConfig.hoistPattern
-    delete pnpmConfig.publicHoistPattern
-  }
   if (typeof pnpmConfig['color'] === 'boolean') {
     switch (pnpmConfig['color']) {
     case true:

config/config/test/index.ts

@@ -382,7 +382,8 @@ test('convert shamefully-flatten to hoist-pattern=* and warn', async () => {
   ])
 })
 
-test('hoist-pattern is undefined if --no-hoist used', async () => {
+// hoist → hoistPattern processing is done in @pnpm/cli-utils
+test('hoist-pattern is unchanged if --no-hoist used', async () => {
   const { config } = await getConfig({
     cliOptions: {
       hoist: false,
@@ -393,7 +394,8 @@ test('hoist-pattern is undefined if --no-hoist used', async () => {
     },
   })
 
-  expect(config.hoistPattern).toBeUndefined()
+  expect(config.hoist).toBe(false)
+  expect(config.hoistPattern).toStrictEqual(['*'])
 })
 
 test('throw error if --no-hoist is used with --shamefully-hoist', async () => {
@@ -444,6 +446,7 @@ test('throw error if --no-hoist is used with --hoist-pattern', async () => {
   })
 })
 
+// public-hoist-pattern normalization is done in @pnpm/cli-utils
 test('normalizing the value of public-hoist-pattern', async () => {
   {
     const { config } = await getConfig({
@@ -456,7 +459,7 @@ test('normalizing the value of public-hoist-pattern', async () => {
       },
     })
 
-    expect(config.publicHoistPattern).toBeUndefined()
+    expect(config.publicHoistPattern).toBe('')
   }
   {
     const { config } = await getConfig({
@@ -469,7 +472,7 @@ test('normalizing the value of public-hoist-pattern', async () => {
       },
     })
 
-    expect(config.publicHoistPattern).toBeUndefined()
+    expect(config.publicHoistPattern).toStrictEqual([''])
   }
 })
 
@@ -1108,6 +1111,7 @@ test('settings sharedWorkspaceLockfile in pnpm-workspace.yaml should take effect
   expect(config.lockfileDir).toBe(undefined)
 })
 
+// shamefullyHoist → publicHoistPattern conversion is done in @pnpm/cli-utils
 test('settings shamefullyHoist in pnpm-workspace.yaml should take effect', async () => {
   const workspaceDir = f.find('settings-in-workspace-yaml')
   process.chdir(workspaceDir)
@@ -1121,7 +1125,6 @@ test('settings shamefullyHoist in pnpm-workspace.yaml should take effect', async
   })
 
   expect(config.shamefullyHoist).toBe(true)
-  expect(config.publicHoistPattern).toStrictEqual(['*'])
   expect(config.rawConfig['shamefully-hoist']).toBe(true)
 })