-
Couldn't load subscription status.
- Fork 50
fix: allow node_modules removal in build phase #259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
a9511f5 to
4a3866e
Compare
814e1f4 to
98f6c39
Compare
98f6c39 to
955d8ff
Compare
| // 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`) |
There was a problem hiding this comment.
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.
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