Skip to content
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

Merged
merged 18 commits into from
May 23, 2024
Merged

fix(ci): rm workspace node_modules #7490

merged 18 commits into from
May 23, 2024

Conversation

reggi
Copy link
Contributor

@reggi reggi commented May 8, 2024

Fixes #7226, an issue where the npm ci needs to remove node_modules within workspace directories.

lib/base-cmd.js Outdated Show resolved Hide resolved
@npm-cli-bot
Copy link
Collaborator

npm-cli-bot commented May 13, 2024

no statistically significant performance changes detected

timing results
app-large clean lock-only cache-only modules-only no-lock no-cache no-modules no-clean show-version run-script cache-only
peer-deps
no-clean
audit
npm@latest 31.277 ±0.66 10.773 ±0.03 11.725 ±0.07 1.533 ±0.02 1.543 ±0.00 1.271 ±0.01 8.303 ±0.01 1.261 ±0.01 0.137 ±0.00 0.169 ±0.01 14.928 ±0.17 2.096 ±0.00
#7490 33.998 ±2.64 10.665 ±0.03 12.338 ±0.37 1.604 ±0.05 1.549 ±0.01 1.294 ±0.01 8.403 ±0.01 1.276 ±0.01 0.137 ±0.00 0.164 ±0.00 14.636 ±0.20 2.116 ±0.04
app-medium clean lock-only cache-only modules-only no-lock no-cache no-modules no-clean show-version run-script cache-only
peer-deps
no-clean
audit
npm@latest 24.931 ±0.50 8.021 ±0.01 8.891 ±0.04 1.493 ±0.01 1.490 ±0.01 1.395 ±0.01 5.771 ±0.05 1.295 ±0.01 0.137 ±0.00 0.164 ±0.00 9.952 ±0.02 1.993 ±0.08
#7490 25.700 ±2.44 8.172 ±0.13 8.972 ±0.06 1.524 ±0.00 1.540 ±0.03 1.403 ±0.01 5.843 ±0.02 1.291 ±0.01 0.138 ±0.00 0.164 ±0.00 9.932 ±0.05 2.053 ±0.10

lib/commands/ci.js Outdated Show resolved Hide resolved
lib/commands/ci.js Outdated Show resolved Hide resolved
lib/commands/ci.js Outdated Show resolved Hide resolved
lib/commands/ci.js Outdated Show resolved Hide resolved
@reggi reggi changed the title fix(ci): rm workspace node_modules fix(ci): rm workspace node_modules May 14, 2024
@reggi reggi marked this pull request as ready for review May 14, 2024 17:03
@reggi reggi requested a review from a team as a code owner May 14, 2024 17:03
Copy link
Contributor

@lukekarrys lukekarrys left a 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.

lib/commands/ci.js Show resolved Hide resolved
lib/commands/ci.js Outdated Show resolved Hide resolved
lib/commands/ci.js Outdated Show resolved Hide resolved
lib/commands/ci.js Outdated Show resolved Hide resolved
lib/commands/ci.js Outdated Show resolved Hide resolved
@reggi reggi merged commit 7d89b55 into latest May 23, 2024
39 checks passed
@reggi reggi deleted the reggi/ci-rm-ws-modules branch May 23, 2024 17:33
@github-actions github-actions bot mentioned this pull request May 23, 2024
@leppaott
Copy link

leppaott commented Jul 29, 2024

This seems now inconsistent behavior #3598
Why would ci prune workspaces when it doesn't install them either? It only prunes what it installs back - that is packages not linking. This broke our system as can't first install the workspace to cache it on docker layer and then install the parent after which we fail to run a command on the workspace since its modules are gone...

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 --workspaces flag?

workspaces
Default: null
Type: null or Boolean
Set to true to run the command in the context of all configured workspaces.

Explicitly setting this to false will cause commands like install to ignore workspaces altogether. When not set explicitly:

Commands that operate on the node_modules tree (install, update, etc.) will link workspaces into the node_modules folder. - Commands that do other things (test, exec, publish, etc.) will operate on the root project, unless one or more workspaces are specified in the workspace config.

Explicitly setting this to false will cause commands like install to ignore workspaces altogether.

At least this doesn't seem to affect this at all? --workspaces=false

@reggi @wraithgar

Seems postinstall is run on ci but it can be excluded with --workspaces for the nested workspace.

You can test my issue of having let say copyfiles package on postinstall for a workspace module.

And you should get following when running npm ci for the parent module:

npm error sh: 1: copyfiles: not found

while package.json looks something like:

"scripts": {
    "postinstall": "copyfiles . .",
  },
  "dependencies": {
    "copyfiles": "^2.4.1",

this shouldn't happen since the workspace module is installed earlier... But now npm ci breaks it...

@wraithgar
Copy link
Member

@leppaott please do not @ notify a bunch of people like this, not everyone you just sent a notification to is even on the npm cli team.

I could not fully understand what the problem was from just your comment. If you think there is a bug in how npm ci works please open up a new issue so we can triage it properly.

@leppaott
Copy link

leppaott commented Jul 30, 2024

@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 clean part yes but it's not doing the install part on clean-install. I was trying to demonstrate that you cannot run a workspace script that calls a binary after the clean-install which you surely can if you just use install here. If you do a clean-install on the workspace it has the binary, after which you do a clean-install on the root it for some reason wipes the workspace's packages.

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.

[BUG] npm ci does not remove/recreate existing node_modules inside workspaces
6 participants