-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
fix(ci): rm workspace node_modules #7490
Conversation
no statistically significant performance changes detected timing results
|
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.
CI
extends ArboristWorkspaceCmd
which will call this.setWorkspaces()
and then this.exec()
. So we can check if this.workspacePaths
is set and use that instead of calling getWorkspaces
directly. Also ArboristWorkspaceCmd
will always set includeWorkspaceRoot: false
so we need to explicitly include where
.
We need to do this because you can run npm ci -w workspace-a
and have it only install the deps for workspace-a
but you can't guarantee that the root node_modules
won't end up populated. So in fixing this bug we want to make sure that we only rm -rf
the node_modules specified by the workspace config, plus the root.
This seems now inconsistent behavior #3598 Revert please? It's unfortunate any of these commands don't really work with workspaces but this behavior shouldn't be changed IMO. This should use the
At least this doesn't seem to affect this at all? Seems You can test my issue of having let say And you should get following when running
while package.json looks something like:
this shouldn't happen since the workspace module is installed earlier... But now npm ci breaks it... |
@leppaott please do not I could not fully understand what the problem was from just your comment. If you think there is a bug in how |
@wraithgar This should be reverted as it's a breaking change to delete any workspace node_modules. TS compilations need those modules. This should be considered with more thought. It's now doing the |
Fixes #7226, an issue where the
npm ci
needs to removenode_modules
within workspace directories.