Conversation
rigor789
force-pushed
the
refactor/package-manager
branch
from
September 16, 2026 09:13
26c4b45 to
2dd6df5
Compare
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Group the package manager dispatcher, the per-manager implementations (npm, yarn, yarn2, pnpm, bun), their shared base class and the installation manager under a single directory, and shorten the implementation class names to match their file names.
Replace the untyped npm flag bag passed to install/uninstall with IPackageInstallOptions and IPackageUninstallOptions (save, dev, optional, exact, silent, ignoreScripts plus the CLI-internal options). Each package manager declares how it spells each option, and options a manager has no flag for are dropped instead of leaking npm syntax onto its command line. This fixes yarn berry receiving --save-dev / --save-exact (silently dropped, so platforms landed in dependencies) and --ignore-scripts (an unknown option that aborted the install); it now gets --dev, --exact and --mode=skip-build. bun receives its own --dev / --exact instead of npm's. Also settle the implementation class names on NpmPackageManager, YarnPackageManager, Yarn2PackageManager, PnpmPackageManager and BunPackageManager.
…ookups view() now takes an optional registry field instead of an npm flag bag, and search() takes only the keywords; each package manager spells the field selection itself (yarn berry uses --fields). The generic npm flag-string builder is gone with them, so nothing in the package manager layer emits npm syntax on behalf of another manager any more. Project-facing package lookups go through Node resolution from the project directory instead of joining node_modules/<name> by hand: test-init peer dependency discovery, the installed path returned by PackageInstallationManager, the local inspector check, the core modules short-import scan in doctor, and the installed core modules versions in versions-service. plugins-service no longer creates an empty node_modules directory before enumerating dependencies.
rigor789
force-pushed
the
refactor/package-manager
branch
from
September 16, 2026 09:40
4904ab7 to
f3f4e1b
Compare
rigor789
changed the base branch from
main
to
feat/define-command-migration
September 16, 2026 09:40
…e manager Add getInstalledPackagePath(packageName, fromDir) to the package manager contract. The base implementation walks node_modules the way Node does; a package manager with a different on-disk layout can override it. Services whose call chains are already async now ask the package manager where a package lives instead of resolving it themselves: plugins-service, doctor short-import scan, versions-service, prepare-controller's runtime package.json lookup, android-plugin-build-service's local gradle versions, the preview command, test-init and PackageInstallationManager. Sites reached only from synchronous code (getRuntimePackage in project-data-service, the bundler executable lookup, the vitest and karma readiness checks, the transitive walk in node-modules-dependencies-builder) keep using the resolution helper directly.
… package manager The bundler executable lookup, the vitest and karma readiness checks and the extensibility service's "is this extension installed?" check now ask the package manager where a package lives. Their tests stub that one method instead of faking directory listings or patching Node's module resolution. getRuntimePackage in project-data-service and the transitive walk in node-modules-dependencies-builder stay on the resolution helper: both feed synchronous code paths (getPlatformData, getAllProductionPlugins) with dozens of callers, and threading async through those is a separate change.
…ckages synchronously Nothing about locating an installed package is asynchronous; the only async link was the dispatcher reading the "packageManager" user setting through the settings lock. JsonFileSettingsService gains a lock-free getSettingValueSync for settings that only change through explicit user commands, and the dispatcher now picks its implementation lazily and synchronously, dropping the @cache/@invokeInit init dance. getInstalledPackagePath is therefore synchronous on the contract, which unwinds the async that had been threaded through doctor, plugins-service, the bundler, the test runners, preview and android-plugin-build-service, and lets the last two direct users of the resolution helper move onto the contract: getRuntimePackage in project-data-service (resolved lazily via the injector, as the service is constructed everywhere) and the transitive walk in node-modules-dependencies-builder.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Steps towards fully abstracting package management in the CLI. The end goal is that the CLI stops caring what
node_modulesorpackage.jsonfiles are (to an extent) and talks to a package-manager-agnostic API, with each package manager mapping that API onto its own flags and terminology. A related goal is full pnpm support without--shamefully-hoist.Stacked on #6153 (
feat/define-command-migration) since that changes the command and$optionssetup.Commit 1: move into
lib/package-managers/The dispatcher, the npm / yarn / yarn2 / pnpm / bun implementations, the shared base class and
PackageInstallationManagernow live inlib/package-managers/. DI bootstrap and tests updated. No behaviour changes.Commit 2: typed install/uninstall options
installanduninstalltakeIPackageInstallOptions/IPackageUninstallOptionsinstead of an untyped bag of npm flags:save: true--save--savesave: false--no-save--no-savedev--save-dev--dev--dev--save-dev--devoptional--save-optional--optional--optional--save-optional--optionalexact--save-exact--exact--exact--save-exact--exactsilent--silent--silent--silent--silent--silentignoreScripts--ignore-scripts--ignore-scripts--mode=skip-build--ignore-scripts--ignore-scriptsEach manager declares its spelling in a table on the class; the base class does the mapping and drops anything a manager has no flag for. This fixes yarn berry silently dropping
--save-dev/--save-exact(platforms ended up independencies) and rejecting--ignore-scripts, and bun being handed npm's spellings.test/package-manager-flags.tspins the table.Classes are
NpmPackageManager,YarnPackageManager,Yarn2PackageManager,PnpmPackageManager,BunPackageManager.Commit 3: manager-agnostic
view/searchview(packageName, field?)takes an optional registry field (versions,dist-tags, ...) instead of{ "dist-tags": true };search(keywords)takes only keywords. yarn berry spells the field as--fields. The generic npm flag-string builder is gone, so the package manager layer no longer emits npm syntax on behalf of another manager anywhere.Commit 4:
getInstalledPackagePathon the contract"Where is package X installed?" is now a question services ask the package manager. The base implementation walks
node_modulesthe way Node does; a manager with a different layout (yarn Plug'n'Play, for instance) can override it, and tests stub one method instead of faking a filesystem.Migrated: plugins-service, doctor's short-import scan, versions-service, prepare-controller's runtime lookup, android-plugin-build-service's local gradle versions, the preview command, test-init, and
PackageInstallationManager.Commit 5: bundler, test runners and extensions on the contract
The bundler executable lookup, the vitest and karma readiness checks, and the extensibility service's "is this extension installed?" check go through
getInstalledPackagePath. Their tests stub that one method instead of faking directory listings or monkey-patching Node's module resolution.Commit 6: synchronous selection and resolution
Nothing about locating an installed package is asynchronous; the only async link was the dispatcher reading the
packageManageruser setting through the settings lock.JsonFileSettingsServicegains a lock-freegetSettingValueSync, the dispatcher selects its implementation lazily and synchronously (no more@cache/@invokeInit), andgetInstalledPackagePathis a plain synchronous method on the contract. That unwinds theasyncearlier commits had threaded through services, and lets the last two direct users of the resolution helper move onto the contract:getRuntimePackagein project-data-service and the transitive walk in node-modules-dependencies-builder. No service resolves packages on its own any more.Still hardcoding
node_modules, on purposeclean,migrate,update, log source maps, watch app resources) genuinely target the directory.node_modulesjoin after the contract confirms it is installed. Every supported manager places direct installs at the top level ofnode_modules, so this holds even for pnpm's isolated layout.--shamefully-hoiststays for now: pnpm's isolated layout only breaks transitive resolution, which is driven by the bundler and runtime inside the project rather than by the CLI's own lookups. Dropping the flag needs verification against a real pnpm project.🤖 Generated with Claude Code