Skip to content

fix: allow node_modules removal in build phase#259

Merged
iloveitaly merged 14 commits into
railwayapp:mainfrom
iloveitaly:node-modules-cache
Nov 18, 2025
Merged

fix: allow node_modules removal in build phase#259
iloveitaly merged 14 commits into
railwayapp:mainfrom
iloveitaly:node-modules-cache

Conversation

@iloveitaly

@iloveitaly iloveitaly commented Sep 11, 2025

Copy link
Copy Markdown
Collaborator

I really don't love the solution here, but it's not clear there is a better way:

  • npm ci does not allow you to disable node_modules removal
  • Users include npm ci in the build step, instead of the install step, which cause node_modules to attempt to be removed which then fails the build since it's a read-only mount when used as a cache directory.
  • We could attempt to redirect various caches that use node_modules/.cache to something like ~/.node_modules_cache but each tool uses their own way of configuring the cache path, and attempting to modify all of this tooling feels like it's not worth the squeeze (in addition to be confusing to users).
  • We do need to document how to workaround "system" catches by defining your own railpack.json. I have that in my todo. This will be confusing to users.
  • It seems like there is not a way around the read-only mountpath that buildkit uses for cache directories.
  • Ideally the user could disable adding the node_modules/.cache, but I think it's better to not create more knobs for users to tune and instead (a) try to do the right thing via ugly regex-style stuff like we've done in this PR and (b) enable the user to override everything via a railpack.json if they are outside the golden path.

What I decided on was "post processing" the build plan after it's been merged with the user-provided railpack.json. It's more complex, but if we attempted to omit NODE_MODULES_CACHE in the node provider:

  • If a user supplied a railpack.json with npm ci they'd run into the same issue, which is odd
  • --build-cmd also ran into the same issue

However, a very obvious case still fails:

{
  "scripts": {
    "build": "npm ci"
  }
}

Without writing a web of complex code to analyize the content of build this would fail, and even that's a fools errand since they could shell out to anything that could attempt to rm -rf node_modules.

I still think the complexity here is worth it since it handles the common case and still gives advanced users the option to (a) understand npm ci should run in install and (b) write their own railpack.json to workaround the issue.

fixes #255

@iloveitaly iloveitaly force-pushed the node-modules-cache branch 5 times, most recently from 98f6c39 to 955d8ff Compare September 12, 2025 22:21
Comment thread core/cleanse.go Outdated
// the node_modules cache in those steps.
var (
// Matches "npm ci" with flexible whitespace, using word boundaries
npmCiCommandRegex = regexp.MustCompile(`(?i)\bnpm\s+ci\b`)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure that this is the correct place for this file. Or maybe we should have an abstraction so that language providers can hook into cleansing. It just feels a bit off having node/npm specific stuff in the core/cleanse.go file when up until now all node and language specific logic has been isolated to the provider directories.

@iloveitaly iloveitaly force-pushed the node-modules-cache branch 2 times, most recently from f26d41e to 00a1d97 Compare November 17, 2025 18:17
@iloveitaly

Copy link
Copy Markdown
Collaborator Author

@coffee-cup this one is (finally) ready for final review!

@coffee-cup coffee-cup left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! I think this pattern will work well. I remember in a few places having to check the ctx.Config or something similar somewhere because of the limitation that the config is applied after. This is much nicer

@iloveitaly iloveitaly merged commit 6f811fb into railwayapp:main Nov 18, 2025
173 of 174 checks passed
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.

Build fails if npm install in build command

2 participants