fix: allow node_modules removal in build phase#259
Merged
Conversation
98f6c39 to
955d8ff
Compare
coffee-cup
reviewed
Sep 13, 2025
| // 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`) |
Contributor
There was a problem hiding this comment.
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.
f26d41e to
00a1d97
Compare
Collaborator
Author
|
@coffee-cup this one is (finally) ready for final review! |
coffee-cup
approved these changes
Nov 18, 2025
coffee-cup
left a comment
Contributor
There was a problem hiding this comment.
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
95ee0b8 to
16f710f
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I really don't love the solution here, but it's not clear there is a better way:
npm cidoes not allow you to disablenode_modulesremovalnpm ciin thebuildstep, instead of the install step, which causenode_modulesto attempt to be removed which then fails the build since it's a read-only mount when used as a cache directory.node_modules/.cacheto something like~/.node_modules_cachebut 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).railpack.json. I have that in my todo. This will be confusing to users.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 arailpack.jsonif 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 omitNODE_MODULES_CACHEin the node provider:railpack.jsonwithnpm cithey'd run into the same issue, which is odd--build-cmdalso ran into the same issueHowever, a very obvious case still fails:
{ "scripts": { "build": "npm ci" } }Without writing a web of complex code to analyize the content of
buildthis would fail, and even that's a fools errand since they could shell out to anything that could attempt torm -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 cishould run ininstalland (b) write their ownrailpack.jsonto workaround the issue.fixes #255