Skip to content

fix(deps): make package-lock.json self-consistent - #2853

Merged
angelodlfrtr merged 1 commit into
masterfrom
fix/package-lock
Oct 17, 2026
Merged

angelodlfrtr merged 1 commit into
masterfrom
fix/package-lock

Conversation

@angelodlfrtr

Copy link
Copy Markdown
Contributor

Problem

package-lock.json does not fully pin the dependency tree. On every npm ci, npm has to go back to the registry for package metadata to fill in what the lock leaves undetermined, and it re-resolves four version ranges at install time. A lockfile should make both unnecessary. In practice this means ~830 extra registry requests per install, tarballs that npm cannot verify against the lock for a quarter of the tree, and breakage for tooling that installs strictly from the lockfile (I hit it packaging the Linux client for nixpkgs).

To measure it, count the non-tarball registry requests npm ci makes from an empty cache:

npm ci --ignore-scripts --no-audit --no-update-notifier --cache "$(mktemp -d)" --loglevel=http 2>&1 \
  | grep -E 'GET [0-9]+ https://registry.npmjs.org/[^ ]+' | grep -vc '\.tgz'

On master this prints 830. With this PR it prints 0. (Measured with npm 10.9 on Node 22, the versions the repo declares. --no-audit matters: the audit step fetches metadata for vulnerable packages independently of the lockfile.)

There are three separate defects:

  1. 827 of 3549 entries have no resolved or integrity field. For example node_modules/webpack-cli is only {"version": "4.10.0", "dev": true, "license": "MIT", …}. This is npm install removes resolved and integrity properties from package-lock.json if installed from cache npm/cli#4263: entries written from a warm local cache lose those fields, and later npm install runs never add them back. Each such entry costs a metadata request on every install, and npm has nothing in the lock to verify the tarball against.

  2. Stale nested entries under client/capacitor. client/capacitor/package.json declares webpack-cli ^4.4.0, webpack-dev-server ^4.5.0 and webpack-merge ^5.8.0, but the lock still carries client/capacitor/node_modules/webpack-cli@7.0.2, webpack-dev-server@5.2.3 and webpack-merge@6.0.1, which do not satisfy those ranges. On every npm ci, npm fetches the packuments, re-resolves the workspace to the root copies (4.10.0 / 4.15.2 / 5.10.0) and prunes the nested entries as extraneous. The 7.x/5.x/6.x versions are therefore never installed anywhere; the lock just claims they are. This is what npm ci --loglevel=silly logs on master:

    npm silly fetch manifest webpack-cli@^4.4.0
    npm silly placeDep ROOT webpack-cli@4.10.0 KEEP for: @outline/client-capacitor@0.0.0-debug want: ^4.4.0
    
  3. A leftover src/cordova/plugin entry from before the plugin moved to client/src/cordova/plugin. The directory no longer exists and npm treats the entry as extraneous.

Change

This PR only edits package-lock.json. No versions change, and the installed tree is identical to what npm ci already produces today; the lock now records it instead of leaving npm to rediscover it.

  • Filled in resolved and integrity for the 827 entries with npm-lockfile-fix (-r limits it to entries without resolved, leaving the git dependencies alone).
  • Removed the four stale entries from points 2 and 3, which npm discards on every install anyway.

The diff is generated, so it is easier to reproduce than to read:

git checkout master -- package-lock.json
pipx run npm-lockfile-fix -r package-lock.json
jq 'del(.packages["client/capacitor/node_modules/webpack-cli",
                  "client/capacitor/node_modules/webpack-dev-server",
                  "client/capacitor/node_modules/webpack-merge",
                  "src/cordova/plugin"])' package-lock.json > tmp && mv tmp package-lock.json
git diff --stat   # empty: same result as this PR

Verification

# 0 metadata requests, only tarballs
npm ci --ignore-scripts --no-audit --no-update-notifier --cache "$(mktemp -d)" --loglevel=http 2>&1 \
  | grep -E 'GET [0-9]+ https://registry.npmjs.org/[^ ]+' | grep -vc '\.tgz'

# no version ranges re-resolved at install time
npm ci --ignore-scripts --no-audit --loglevel=silly 2>&1 | grep -c 'fetch manifest'

# builds still work
npm run action client/electron/build_main linux -- --arch=x64

The affected entries only serve the Capacitor workspace, which no release build uses, so the Electron, Cordova and server-manager builds are unaffected.

Notes

  • I chose the minimal fix over regenerating the lockfile from scratch (rm package-lock.json && npm install), because regeneration re-resolves every floating range and moves to lockfileVersion 3, which is a much larger change to review.
  • A follow-up CI check could catch this class of regression: run the metadata-request count above after npm ci and fail if it is non-zero.

🤖 Generated with Claude Code

@angelodlfrtr
angelodlfrtr marked this pull request as ready for review September 15, 2026 11:05
@angelodlfrtr
angelodlfrtr requested a review from a team as a code owner September 15, 2026 11:05
@angelodlfrtr

Copy link
Copy Markdown
Contributor Author

@greptileai

@greptile-apps

greptile-apps Bot commented Sep 15, 2026

Copy link
Copy Markdown

Greptile Summary

The lockfile now fully describes the dependency tree that npm ci already installs. It adds missing package URLs and hashes, and removes stale Capacitor and Cordova entries so npm no longer needs to rediscover the tree from the registry.

  • Pins registry packages with resolved tarball URLs and integrity hashes.
  • Removes three mismatched nested webpack records and one obsolete Cordova record.
  • Keeps the installed package versions unchanged while making the lockfile self-consistent.

Confidence Score: 5/5

The lockfile-only change appears safe to merge.

The PR keeps all dependency versions unchanged. A clean install accepted the new URLs and hashes. Compatible hoisted packages and the current Cordova plugin records remain in the lockfile.

Important Files Changed

Filename Overview
package-lock.json Adds resolved and integrity to 827 existing packages. It removes four stale paths without changing dependency versions. A clean install accepted the new hashes and left the lockfile unchanged.

Sequence Diagram

sequenceDiagram
    participant Dev as Developer or CI
    participant NPM as npm ci
    participant Lock as package-lock.json
    participant Registry as npm registry
    Dev->>NPM: Start clean install
    NPM->>Lock: Read pinned package entries
    Lock-->>NPM: Return versions, tarball URLs, and hashes
    NPM->>Registry: Fetch package tarballs
    Registry-->>NPM: Return tarballs
    NPM->>NPM: Check each tarball hash
    NPM->>NPM: Hoist matching Capacitor dependencies
    NPM-->>Dev: Finish without metadata lookups
Loading

Reviews (1): Last reviewed commit: "fix(deps): make package-lock.json self-c..." | Re-trigger Greptile

@ohnorobo

ohnorobo commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

I just merged

We should also make sure it runs against the un-merged android capacitor CI #2809, and for capacitor iOS #2854

@ohnorobo ohnorobo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nicccce

I agree we should add some CI to keep this in check.
I'm also not opposed to just rming and starting fresh, including the move to lockfileVersion 3

@angelodlfrtr

angelodlfrtr commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor Author

Yes, maybe I might do that in a different PR. This one do not touch the dependency tree at all, removing the lock and changing the version could introduce more changes.

It could also be the occasion to update some other dependencies like electron, koffi, npm audit findings.

@fortuna

fortuna commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

I believe the capacitor pra broke it. The proper fix would be to revert the lock files to before the PRs, then call npm install. That way we preserve the original versions and minimize the change

@ohnorobo

Copy link
Copy Markdown
Contributor

So presumably the issue is one of the capacitor changes here from May 5 to today:
https://github.com/OutlineFoundation/outline-apps/commits/master/package-lock.json

Those are also adding needed deps for capacitor, so we can't strictly revert them. It would be helpful to get the CI additions #2809 and #2854 merged so we can test the regenerated lockfile against those builds.

@ohnorobo

Copy link
Copy Markdown
Contributor

The CI PRs are now merged

The lockfile did not fully pin the dependency tree, so `npm ci` had to
go back to the registry for package metadata on every install:

- 827 entries lacked `resolved` and `integrity` (npm/cli#4263), so npm
  could neither locate nor verify their tarballs from the lock alone.
- client/capacitor/node_modules/webpack-{cli,dev-server,merge} were stale
  copies (7.0.2 / 5.2.3 / 6.0.1) that do not satisfy the ranges declared
  in client/capacitor/package.json; npm re-resolved the workspace to the
  root copies and pruned them on every install.
- src/cordova/plugin was a leftover entry from before the plugin moved to
  client/src/cordova/plugin.

Fill in the missing fields with `npm-lockfile-fix -r` and drop the four
stale entries. No versions change; the installed tree is the one npm
already produced. Metadata requests made by `npm ci` from an empty cache
(with --no-audit, npm 10.9 / Node 22) go from 830 to 0.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@angelodlfrtr

Copy link
Copy Markdown
Contributor Author

@ohnorobo rebased with latest commit from master. I've tried to start fresh but I had too many breaking changes, it should go in another PR. If it is ok for you we can merge.

@angelodlfrtr
angelodlfrtr merged commit 294367c into master Oct 17, 2026
37 of 38 checks passed
@angelodlfrtr
angelodlfrtr deleted the fix/package-lock branch October 17, 2026 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants