Fix pnpm interaction with custom cache #1522
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a `cacheDirectories` field is set in `package.json`, this triggers [custom cache behavior](https://devcenter.heroku.com/articles/nodejs-classic-buildpack-builds#custom-caching) in the buildpack. This is often used with tools that produce their own specialized caches like Next.js which can cache asset compilation results. Since custom cache behavior means the user is taking full control of what needs to be cached, the `node_modules` folder is typically added to the cache directory list to ensure that installed modules are also cached. For npm and Yarn this doesn't present a problem but, for pnpm, [the `node_modules` folder is used differently](https://pnpm.io/motivation#creating-a-non-flat-node_modules-directory) and, if the folder already exists (e.g.; on cache restore), it will cause the build to halt with an `ERR_PNPM_ABORTED_REMOVE_MODULES_DIR_NO_TTY` error. After examining the custom cache code, I'm certain that even how npm and Yarn handle this scenario is incorrect. The user should never cache the `node_modules` folder. What should happen instead is (for default or custom cache behavior), the npm cache, Yarn cache, and pnpm store are what should be always cached unless `NODE_MODULES_CACHE=false`. For now, I'm just going to address this pnpm case though. This PR makes the following changes: ### Custom Cache Behavior with pnpm - if `node_modules` is specified, it will be ignored - all other cache directories will be processed - the pnpm store will be inserted into the custom cache list ### pnpm Reporter A `PNPM_INSTALL_REPORTER` environment variable was introduced to allow for more detailed pnpm output to be produced during install. If set it, the value of this environment variable will be used to append the [`--reporter`](https://pnpm.io/cli/install#--reportername) argument when `pnpm install` is executed. This is used by the tests in this PR to ensure that the pnpm store is functioning correctly, but it could also be useful when debugging builds where pnpm installs are failing unexpectedly.
Signed-off-by: Colin Casey <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a
cacheDirectoriesfield is set inpackage.json, this triggers custom cache behavior in the buildpack. This is often used with tools that produce their own specialized caches like Next.js which can cache asset compilation results.Since custom cache behavior means the user is taking full control of what needs to be cached, the
node_modulesfolder is typically added to the cache directory list to ensure that installed modules are also cached. For npm and Yarn this doesn't present a problem but, for pnpm, thenode_modulesfolder is used differently and, if the folder already exists (e.g.; on cache restore), it will cause the build to halt with anERR_PNPM_ABORTED_REMOVE_MODULES_DIR_NO_TTYerror.After examining the custom cache code, I'm certain that even how npm and Yarn handle this scenario is incorrect. The user should never cache the
node_modulesfolder. What should happen instead is (for default or custom cache behavior), the npm cache, Yarn cache, and pnpm store are what should be always cached unlessNODE_MODULES_CACHE=false. For now, I'm just going to address this pnpm case though. This PR makes the following changes:Custom Cache Behavior with pnpm
node_modulesis specified, it will be ignoredpnpm Reporter
A
PNPM_INSTALL_REPORTERenvironment variable was introduced to allow for more detailed pnpm output to be produced during install. If set it, the value of this environment variable will be used to append the--reporterargument whenpnpm installis executed. This is used by the tests in this PR to ensure that the pnpm store is functioning correctly, but it could also be useful when debugging builds where pnpm installs are failing unexpectedly.W-20494168
Fixes #1518