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

Workspace npm updates are hoisted with mismatched versions #7157

Open
1 task done
colinrotherham opened this issue Apr 24, 2023 · 3 comments
Open
1 task done

Workspace npm updates are hoisted with mismatched versions #7157

colinrotherham opened this issue Apr 24, 2023 · 3 comments
Labels
T: bug 🐞 Something isn't working

Comments

@colinrotherham
Copy link

colinrotherham commented Apr 24, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Package ecosystem

npm

Package manager version

npm 9.6.2

Language version

Node.js 18.14.0

Manifest location and content before the Dependabot update

Repository: https://github.com/alphagov/govuk-frontend

/package-lock.json
/package.json

With npm workspaces:

"workspaces": [
  "app",
  "docs/examples/*",
  "package",
  "shared/*"
],

That have package.json files:

/app/package.json
/docs/examples/webpack/package.json
/shared/config/package.json
/shared/helpers/package.json
/shared/lib/package.json
/shared/tasks/package.json

dependabot.yml content

https://github.com/alphagov/govuk-frontend/blob/main/.github/dependabot.yml

Updated dependency

Bumps express-validator from 6.15.0 to 7.0.1.

What you expected to see, versus what you actually saw

We expected to see a non-hoisted npm package updated in its npm workspace
Instead we saw a partial update, child dependencies hoisting, and old package-lock.json entries remaining

Updating express-validator from 6.15.0 to 7.0.1.

Our current version of express-validator starts off in app/node_modules

Before the update our package-lock.json shows it isn't hoisted:

    "app/node_modules/express-validator": {
      "version": "6.15.0",
      "resolved": "https://registry.npmjs.org/express-validator/-/express-validator-6.15.0.tgz",
      "integrity": "sha512-r05VYoBL3i2pswuehoFSy+uM8NBuVaY7avp5qrYjQBDzagx2Z5A77FZqPT8/gNLF3HopWkIzaTFaC4JysWXLqg==",
      "dependencies": {
        "lodash": "^4.17.21",
        "validator": "^13.9.0"
      },
      "engines": {
        "node": ">= 8.0.0"
      }
    },
    "app/node_modules/validator": {
      "version": "13.9.0",
      "resolved": "https://registry.npmjs.org/validator/-/validator-13.9.0.tgz",
      "integrity": "sha512-B+dGG8U3fdtM0/aNK4/X8CXq/EcxU2WPrPEkJGslb47qyHsxmbggTWK0yEA4qnYVNF+nxNlN88o14hIcPmSIEA==",
      "engines": {
        "node": ">= 0.10"
      }
    },

Maybe it's not hoisted due to peerDependency issues in express-validator dependency validator?

govuk-frontend-repository@ /path/to/project/govuk-frontend
└─┬ govuk-frontend-review@ -> ./app
  └─┬ [email protected]
    └── [email protected]

But in the Dependabot PR we can see the following changes to package-lock.json

diff --git a/package-lock.json b/package-lock.json
index 73957ee24..22f88fe50 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -88,7 +88,7 @@
         "chokidar": "^3.5.3",
         "cookie-parser": "^1.4.6",
         "express": "^4.18.2",
-        "express-validator": "^6.15.0",
+        "express-validator": "^7.0.1",
         "govuk_frontend_toolkit": "^9.0.1",
         "govuk_template_jinja": "^0.26.0",
         "govuk-elements-sass": "3.1.3",
@@ -130,14 +130,6 @@
         "node": ">= 8.0.0"
       }
     },
-    "app/node_modules/validator": {
-      "version": "13.9.0",
-      "resolved": "https://registry.npmjs.org/validator/-/validator-13.9.0.tgz",
-      "integrity": "sha512-B+dGG8U3fdtM0/aNK4/X8CXq/EcxU2WPrPEkJGslb47qyHsxmbggTWK0yEA4qnYVNF+nxNlN88o14hIcPmSIEA==",
-      "engines": {
-        "node": ">= 0.10"
-      }
-    },
     "config": {
       "name": "govuk-frontend-config",
       "extraneous": true,
@@ -8207,15 +8199,6 @@
         "postcss": "^8.2.15"
       }
     },
-    "node_modules/cssnano/node_modules/lilconfig": {
-      "version": "2.1.0",
-      "resolved": "https://registry.npmjs.org/lilconfig/-/lilconfig-2.1.0.tgz",
-      "integrity": "sha512-utWOt/GHzuUxnLKxB6dk81RoOeoNeHgbrXiuGk4yyF5qlRz+iIVWu56E2fqGHFrXz0QNUhLB/8nKqvRH66JKGQ==",
-      "dev": true,
-      "engines": {
-        "node": ">=10"
-      }
-    },
     "node_modules/csso": {
       "version": "5.0.5",
       "resolved": "https://registry.npmjs.org/csso/-/csso-5.0.5.tgz",
@@ -25779,6 +25762,14 @@
         "spdx-expression-parse": "^3.0.0"
       }
     },
+    "node_modules/validator": {
+      "version": "13.9.0",
+      "resolved": "https://registry.npmjs.org/validator/-/validator-13.9.0.tgz",
+      "integrity": "sha512-B+dGG8U3fdtM0/aNK4/X8CXq/EcxU2WPrPEkJGslb47qyHsxmbggTWK0yEA4qnYVNF+nxNlN88o14hIcPmSIEA==",
+      "engines": {
+        "node": ">= 0.10"
+      }
+    },
     "node_modules/value-or-function": {
       "version": "3.0.0",
       "resolved": "https://registry.npmjs.org/value-or-function/-/value-or-function-3.0.0.tgz",

Thankfully we have a postinstall script which runs:

npm ls --depth=0

Which reveals the old [email protected] is still set somewhere:

npm ERR! code ELSPROBLEMS
npm ERR! invalid: [email protected] /home/runner/work/govuk-frontend/govuk-frontend/app/node_modules/express-validator

Taking an extract from the tree view we can see it again here:

├─┬ govuk-frontend-review@ -> ./app
│ ├── [email protected] deduped
│ ├── [email protected]
│ ├── [email protected]
│ ├── [email protected] invalid: "^7.0.1" from app
│ ├── [email protected]

☝️ This is the problem

The version is bumped, a child dependency is hoisted, but the workspace node_modules entry remains

Native package manager behavior

The native package manager npm also has issues updating this package

But we have found some workarounds that Dependabot could also use?

These are still issues in [email protected]

❌ Package not updated but is hoisted

npm install [email protected] --workspace app --save

✅ Package updated and is hoisted

npm install [email protected] --workspace app --save
npm update express-validator --workspace app --save

✅ Package removed, updated and is hoisted

npm uninstall express-validator --workspace app
npm install [email protected] --workspace app --save

Where that last workaround shows that npm correctly hoists express-validator to node_modules:

diff --git a/package-lock.json b/package-lock.json
index 22f88fe50..b4babbbcb 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -118,18 +118,6 @@
         "npm": "^8.1.0 || ^9.1.0"
       }
     },
-    "app/node_modules/express-validator": {
-      "version": "6.15.0",
-      "resolved": "https://registry.npmjs.org/express-validator/-/express-validator-6.15.0.tgz",
-      "integrity": "sha512-r05VYoBL3i2pswuehoFSy+uM8NBuVaY7avp5qrYjQBDzagx2Z5A77FZqPT8/gNLF3HopWkIzaTFaC4JysWXLqg==",
-      "dependencies": {
-        "lodash": "^4.17.21",
-        "validator": "^13.9.0"
-      },
-      "engines": {
-        "node": ">= 8.0.0"
-      }
-    },
     "config": {
       "name": "govuk-frontend-config",
       "extraneous": true,
@@ -10522,6 +10510,18 @@
         "node": ">= 0.10.0"
       }
     },
+    "node_modules/express-validator": {
+      "version": "7.0.1",
+      "resolved": "https://registry.npmjs.org/express-validator/-/express-validator-7.0.1.tgz",
+      "integrity": "sha512-oB+z9QOzQIE8FnlINqyIFA8eIckahC6qc8KtqLdLJcU3/phVyuhXH3bA4qzcrhme+1RYaCSwrq+TlZ/kAKIARA==",
+      "dependencies": {
+        "lodash": "^4.17.21",
+        "validator": "^13.9.0"
+      },
+      "engines": {
+        "node": ">= 8.0.0"
+      }
+    },
     "node_modules/express/node_modules/cookie": {
       "version": "0.5.0",
       "resolved": "https://registry.npmjs.org/cookie/-/cookie-0.5.0.tgz",

Images of the diff or a link to the PR, issue, or logs

No response

Smallest manifest that reproduces the issue

No response

@colinrotherham
Copy link
Author

I've referenced a few other PR that had the same issue

@jenseng
Copy link

jenseng commented Nov 27, 2023

The hoisting and tree consistency issues are likely due to npm/cli#7018 and npm/cli#7019

@colinrotherham
Copy link
Author

@jenseng I'm happy with that if it's out of Dependabot's control, thanks for those links

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: bug 🐞 Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants