Skip to content

Fix pnpm config environment variables to support pnpm v11#1625

Open
colincasey wants to merge 6 commits intomainfrom
ccasey/pnpm-11-support
Open

Fix pnpm config environment variables to support pnpm v11#1625
colincasey wants to merge 6 commits intomainfrom
ccasey/pnpm-11-support

Conversation

@colincasey
Copy link
Copy Markdown
Contributor

Summary

  • Replace pnpm config set store-dir with npm_config_store_dir env var for configuring the pnpm content-addressable store location
  • The env var approach works across all pnpm versions (7.x – 11.x) and avoids version-specific config key naming (store-dir vs storeDir) and --location flag issues introduced in pnpm 11
  • Add hatchet test fixtures and integration tests for pnpm 10 and 11 (both workspace and non-workspace apps)

W-22278392

Replace `pnpm config set store-dir` with `npm_config_store_dir` env
var for configuring the pnpm content-addressable store location. The
env var approach works across all pnpm versions (7.x - 11.x) and
avoids version-specific config key naming (store-dir vs storeDir) and
--location flag issues introduced in pnpm 11.

Add hatchet test fixtures and integration tests for pnpm 10 and 11
(both workspace and non-workspace apps).
@colincasey colincasey self-assigned this Apr 29, 2026
@colincasey colincasey marked this pull request as ready for review April 29, 2026 17:16
@colincasey colincasey requested a review from a team as a code owner April 29, 2026 17:16
@colincasey colincasey enabled auto-merge (squash) April 29, 2026 17:32
Comment thread bin/compile Outdated
build_data::set_duration "install_pnpm_binary_time" "$install_start"
pnpm config set store-dir "$PNPM_CONFIG_CACHE" 2>&1
# npm_config_store_dir works across all pnpm versions (7.x – 11.x)
export npm_config_store_dir="$PNPM_CONFIG_CACHE"
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.

Since this is set in a subshell, it won't be propagated to later steps that run outside of install_bins.

Claude has this to say:

PR Review Summary — #1625

🚨 Critical Issues

1. The fix is a no-op — export dies in a pipelined subshell

File: bin/compile:347 (inside install_bins() defined at L208, called at L384)

install_bins | output::indent | tee -a "$LOG_FILE"   # ← pipeline

In bash, each pipeline stage runs in its own subshell. The export npm_config_store_dir=... on line 347 only affects the subshell and is discarded when install_bins returns. By the time pnpm_install runs later in the parent shell (via build_dependencieslib/dependencies.sh:326), $npm_config_store_dir is unset. pnpm then writes its store to the default location (~/.local/share/pnpm/store), not $PNPM_CONFIG_CACHE.

Why this is worse than before: The old pnpm config set store-dir wrote to a config file on disk, which did survive subshells. The replacement loses that persistence. The existing integration tests cannot catch this (see Critical #2).

Suggested fix: Move the export to the top-level script body next to the existing export PNPM_CONFIG_CACHE at bin/compile:206, e.g.:

export YARN_CACHE_FOLDER NPM_CONFIG_CACHE PNPM_CONFIG_CACHE npm_config_store_dir=$PNPM_CONFIG_CACHE

…or set it inline when invoking pnpm in lib/dependencies.sh.

2. Tests pass even when the fix is broken

File: spec/ci/pnpm_spec.rb (all 4 new specs)

The tests assert "Restoring cache" and "- pnpm cache", but those strings are printed unconditionally in lib/cache.sh:75/:182 whenever the cache directory exists (it always does — created via mktemp at bin/compile:205). Because $PNPM_CONFIG_CACHE gets created empty, mv succeeds, the log line prints, and the test passes — even with the bug in Critical #1 present, and even in the current state of this PR.

Suggested fix: Add an assertion that pnpm actually reused the store on rebuild. Options:

  • Log line like Progress: resolved X, reused N with N > 0
  • Compare pnpm store path output to $PNPM_CONFIG_CACHE
  • Post-install check that $PNPM_CONFIG_CACHE is non-empty

⚠️ Important Issues

3. Fixture inconsistency between pnpm-10-workspace and pnpm-11-workspace

  • pnpm-10-workspace/packages/client: single console.log('client'), depends only on express.
  • pnpm-11-workspace/packages/client: uses lodash, depends on lodash/@types/lodash/prettier.

The tests assert identical server behavior — the client divergence isn't required for any assertion. Align the two fixtures (the existing pnpm-workspace/ fixture matches the pnpm-11 variant, so pnpm-10 appears to be an accidental divergence).

4. Lost failure signal

The old pnpm config set returned non-zero if the key was rejected; the new export cannot fail. If pnpm ever deprecates its npm_config_* shim, this buildpack will silently regress. Consider a post-install sanity check (e.g., pnpm store path equals $PNPM_CONFIG_CACHE) — this would also catch Critical #1.

5. CHANGELOG wording is vague

CHANGELOG.md:5: "Fix pnpm config environment variables to support pnpm v11" is misleading — there were no prior env vars to "fix." Suggest: "Use npm_config_store_dir env var instead of pnpm config set store-dir so the pnpm store cache works with pnpm v11."


💡 Suggestions

  • Comment at bin/compile:346 — explains what (version range) but not why (pnpm 11 --location flag + store-dir/storeDir naming). The version range "7.x – 11.x" will rot. Prefer a rationale-focused comment with a link to the upstream pnpm issue.
  • Tighten regexUsing pnpm .+ at lines 51/83/114/146 would still match pnpm 9 if version selection silently failed. Prefer Using pnpm 11\.\d+\.\d+.
  • Parameterize duplicated specs — the 4 new it blocks are near-identical; an it_behaves_like "pnpm version", "11.0.0", "pnpm-11" pattern shrinks the file and prevents drift.

Assert that pnpm reports reused packages on rebuild to verify
the store-dir cache is actually being used.
install_bins runs in a pipeline (install_bins | output::indent | tee)
so exports inside it are lost when the subshell exits. Move the export
to the top-level script alongside the other cache dir exports.
pnpm <11 reads npm_config_*, pnpm 11+ reads pnpm_config_*.
Set both to cover all versions without version detection.
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.

2 participants