Repository navigation
refactor(pm): pipeline deps resolver - #2576
Conversation
Summary of ChangesHello @elrrrrrrr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the dependency resolution and installation process within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring to implement a pipelined dependency installation process, which should greatly improve performance by parallelizing downloads and installations. The introduction of the pipeline service and the use of OnceMap for deduplicating work are excellent changes. However, I have identified a few areas for improvement. The switch to #[tokio::main] removes some potentially important runtime performance tuning. Additionally, the new download and extraction logic in downloader.rs has moved from a streaming to an in-memory model, which could cause high memory usage and potential OOM errors for large packages. I've also suggested a minor structural improvement in the same file to combine blocking operations.
78745c2 to
8ad00c2
Compare
2f66e20 to
70be474
Compare
70be474 to
48350dd
Compare
#2576 replaced `ensure_package_lock(root_path)` with a bare `fs::try_exists(&lock_path)` check in Self::install. As a result any `ut install` with an existing package-lock.json skipped the manifest resolution entirely — even when package.json had been edited — silently installing the stale tree from the lockfile. Re-introduce the freshness check: when the lockfile exists we now still run `is_pkg_lock_outdated`. Stale lockfiles (or freshness-check errors — corrupt lockfiles etc.) fall through to the pipeline cold path so we keep the concurrent resolve + download benefit #2576 was built for. Regression guard lives in `e2e/pm/stale-lockfile/` — a fixture that commits a package-lock.json whose root `dependencies` is empty while package.json declares abbrev and ini. Case 8.2 in e2e/utoo-pm.sh asserts: - `ut install` regenerates the lockfile - both packages end up in node_modules/ Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
`fs::try_exists(&lock_path)` check in Self::install. As a result any `ut install` with an existing package-lock.json skipped the manifest resolution entirely — even when package.json had been edited — silently installing the stale tree from the lockfile. Re-introduce the freshness check: when the lockfile exists we now still run `is_pkg_lock_outdated`. Stale lockfiles (or freshness-check errors — corrupt lockfiles etc.) fall through to the pipeline cold path so we keep the concurrent resolve + download benefit #2576 was built for. Regression guard lives in `e2e/pm/stale-lockfile/` — a fixture that commits a package-lock.json whose root `dependencies` is empty while package.json declares abbrev and ini. Case 8.2 in e2e/utoo-pm.sh asserts: - `ut install` regenerates the lockfile - both packages end up in node_modules/ Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(pm): detect stale lockfile in ut install `fs::try_exists(&lock_path)` check in Self::install. As a result any `ut install` with an existing package-lock.json skipped the manifest resolution entirely — even when package.json had been edited — silently installing the stale tree from the lockfile. Re-introduce the freshness check: when the lockfile exists we now still run `is_pkg_lock_outdated`. Stale lockfiles (or freshness-check errors — corrupt lockfiles etc.) fall through to the pipeline cold path so we keep the concurrent resolve + download benefit #2576 was built for. Regression guard lives in `e2e/pm/stale-lockfile/` — a fixture that commits a package-lock.json whose root `dependencies` is empty while package.json declares abbrev and ini. Case 8.2 in e2e/utoo-pm.sh asserts: - `ut install` regenerates the lockfile - both packages end up in node_modules/ Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(pm): simplify stale-lockfile detection per review - Drop duplicate `tracing::warn` in Self::install; `is_pkg_lock_outdated` already logs the specific mismatch reason (root deps changed, engines changed, …). Users now see one warn instead of two per stale detection. - Collapse `lock_exists` + `lock_stale` into a single `use_fresh_lock` expression — removes the double-negation. - Drop the committed stale `package-lock.json` fixture and the `git checkout --` dance in Case 8.2. The e2e now writes the stale lockfile inline via heredoc, which: * survives local edits / worktrees / stashed state * avoids the `git add -f` fight with .gitignore * keeps the fixture dir to just package.json (the declarative half) - Assert root deps via `node` inspection of `packages[""].dependencies` instead of `grep "abbrev"`, which would match transitive `node_modules/abbrev` entries and hide a regression where only the tree — not the root declaration — got rewritten. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * perf(pm): reuse cached package.json in is_pkg_lock_outdated The freshness check was reading the root package.json three times per `ut install` — once as DepsView, once inside find_workspaces (for the "workspaces" field), once as EnginesView. Each workspace was also read twice (once by find_workspaces, again as DepsView). Load the root as full `PackageJson` via `get_or_load_package_json` so subsequent reads on the same path hit the in-process cache (filled by `update_package_json`'s write-through). Within this function, drop the DepsView/EnginesView intermediates and read the fields directly off the same `PackageJson`; workspaces come back pre-loaded on the `find_workspaces` tuple so we don't re-load them either. Cache coherence: `update_package_json` already calls `set_package_json` write-through after every edit, and every `ut install` starts a fresh process with a cold cache — so external edits (IDE, `git checkout`) still see a disk read next run. Tests: the original `test_is_pkg_lock_outdated` simulated a "write file, re-check" flow that doesn't happen in real invocations (every mutation goes via `update_package_json` + write-through). Split it into five independent scenarios, each with its own TempDir so cache keys don't collide, which is also the idiomatic "one test per case". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
oncemapfor manifest parsing, TGZ download, extraction, and file cloning.libdeflaterfor decompression andrayonfor parallel file writing.tokio-runtimespawn_blocking limitations.