feat(npm): add catalog: protocol for centralized dependency versions in workspaces#32947
Conversation
…s in workspaces Adds support for the `catalog:` protocol in workspace member `package.json` files, allowing monorepos to define dependency versions centrally in the root `deno.json` and reference them via `"pkg": "catalog:"` in members. This is similar to pnpm/Bun catalog support and eliminates version drift across workspace members. Closes #30403 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Replace misleading url::ParseError::EmptyHost with a dedicated CatalogPackageNotFound error variant for clear error messages - Add spec test for missing catalog entry error case Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
lunadogbot
left a comment
lunadogbot
left a comment
There was a problem hiding this comment.
Read the resolution paths (cli/args/mod.rs, cli/rt/run.rs, cli/tools/info.rs, cli/tools/publish/unfurl.rs), the workspace plumbing (libs/config/workspace, libs/resolver/workspace, libs/resolver/lib), the lockfile changes, BYONM resolution, and the install categorizer. The shape is consistent across all six paths -- Catalog is a unit variant, version is sourced from the workspace root's catalog map, and the resolved npm: URL is built the same way each time. Three concerns I'd want explicit decisions on before merging -- all silent-fallback footguns rather than crash bugs:
1. Named catalogs silently coerce to the default -- in libs/package_json/lib.rs, the parser arm "catalog" => Ok(Self::Catalog) ignores everything after the colon, so "catalog:", "catalog:default", and "catalog:react18" all parse to the same PackageJsonDepValue::Catalog and pick up the default catalog. pnpm and Bun both treat named catalogs as the primary use case; if Deno's intent is default-only for v1, capturing the suffix in the variant (Catalog(Option<String>)) and rejecting non-empty suffixes at parse time would prevent users from typing catalog:react18 and silently getting whatever react18 doesn't refer to. The PR test only covers "catalog:" (lib.rs:875).
2. resolve_catalog_dep returns None for two distinct conditions -- (a) the alias isn't in the catalog map, (b) the alias is in the catalog but its version_req string fails VersionReq::parse_from_npm. The callers in cli/args/mod.rs, cli/rt/run.rs, and libs/resolver/lib.rs (CatalogPackageNotFound) all surface case (b) as "Package '{}' not found in catalog", which is confusing for someone who can see the package in deno.json -- they'll think the catalog isn't being read. Worth either propagating the parse error or emitting a workspace diagnostic at config-load time.
3. cli/tools/installer/local.rs insertion is unconditional on parse success -- the new Catalog branch in categorize_installed_npm_deps does if workspace.catalog().contains_key(k.as_str()) { normal_deps.insert(k.to_string()); } without parsing the version_req. If the catalog has "@foo/bar": "not-a-version", this still adds @foo/bar to the install set, then resolution silently fails downstream with the misleading "not found in catalog" message. Either gate on a successful parse here or surface the bad version_req as a WorkspaceDiagnostic at load time.
The two new spec tests (catalog_deps, catalog_deps_missing) cover the happy path and the missing-key path -- neither hits any of the above. Not requesting changes since these are policy decisions you may have already made; flagging so they're explicit.
- Reject named catalog suffixes (`catalog:react18`) at parse time. Only accept `catalog:` and `catalog:default`; previously any suffix silently coerced to the default catalog and pnpm/Bun users could expect named-catalog semantics. - Gate installer insertion on a successful version_req parse via `resolve_catalog_dep().is_some()` instead of plain `contains_key()`, so a typo in the catalog version surfaces consistently rather than flowing into the install set and failing later with a misleading "not found in catalog" message. - Emit `WorkspaceDiagnostic::InvalidCatalogVersionReq` at config-load when a catalog entry's version_req fails to parse, so the user sees a single clear message rather than the downstream resolution error.
|
Follow-up to my prior review: I rebased this branch on Branch: Two commits on top of
Happy to drop or rework any of this if you'd rather keep the v1 surface narrower. |
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
Substantive review by @lunadogbot covers the resolution paths thoroughly. Reinforcing two of their three points as I'd argue blocker-grade rather than "policy decisions" — the named-catalog silent coercion and the parse-error-as-not-found are both deterministic footguns where the user gets the wrong package version with no diagnostic. Adding inline notes pinning each to its line.
CI
133 pass, 2 fail, 14 skipping. The two fails are ci status (rollup) and test node_compat (1/3) debug windows-x86_64 on test-dns-resolver-max-timeout.js (timeout1: 504, timeout2: 516) — that's the recurring DNS-resolver-timing flake on Windows debug, completely unrelated to the catalog feature.
Holding COMMENT rather than approving — this is a new public protocol (catalog: in package.json) plus a new catalog config-file root key, which is escalation territory regardless of CI state.
| from_name_and_version_req(value.into(), "*") | ||
| } | ||
| } | ||
| "catalog" => Ok(Self::Catalog), |
There was a problem hiding this comment.
Named catalogs silently coerce to default.
"catalog" => Ok(Self::Catalog),The match arm ignores everything after :. So "catalog:", "catalog:default", and "catalog:react18" all parse to the same PackageJsonDepValue::Catalog and look up the alias in the default catalog map.
Real failure mode: a user copy-pastes a pnpm-style package.json into a Deno workspace:
// pnpm-style: looks up react18 named catalog
{ "react": "catalog:react18" }Deno silently parses this as default-catalog lookup. If "react18" and "react" resolve to different versions in the user's mental model (because they were named catalogs in the original repo), the user gets the wrong React version with no diagnostic. pnpm and Bun both treat named catalogs as the primary use case.
If default-only is the v1 scope, capture the suffix and reject non-empty:
"catalog" => {
if !value.is_empty() {
return Err(PackageJsonDepValueParseErrorKind::Unsupported {
scheme: format!("catalog:{value}"),
});
}
Ok(Self::Catalog)
}Or capture as Self::Catalog(Option<String>) and surface a diagnostic with a list of available catalogs (as a foothold for future named-catalog support). Either prevents the silent wrong-version footgun.
The unit test below at line ~875 ("catalog-test" → "catalog:test") is currently asserting the bug — after the fix it should expect Err.
| .workspace_resolver | ||
| .resolve_catalog_dep(alias) | ||
| .ok_or_else(|| { | ||
| DenoResolveErrorKind::CatalogPackageNotFound( |
There was a problem hiding this comment.
Reinforcing @lunadogbot's #2: CatalogPackageNotFound here collapses two distinct conditions into the same downstream error:
- Alias missing from catalog map — user typoed the package name, expected: "
reactnot found in catalog". - Alias present but version_req fails
VersionReq::parse_from_npm— user typed a malformed version ("^18.x"instead of"^18.0.0"). Same error message: "reactnot found in catalog". Confusing — the user can see the entry indeno.json.
Same pattern in libs/resolver/lockfile.rs:303–313:
PackageJsonDepValue::Catalog => {
let version_req_str = catalog.get(alias.as_str())?;
let version_req = VersionReq::parse_from_npm(version_req_str).ok()?;
// ...
}If the version doesn't parse, .ok()? silently drops the dep from the lockfile's workspace deps — the dep just isn't in the lockfile, but the install path may still try to install it from somewhere else, leading to inconsistent state.
Fix: emit a WorkspaceDiagnostic at config-load time when any catalog entry has an unparseable version, so the user sees the error early rather than at install resolution time. Keep the None short-circuit at the resolver call sites, but only because the diagnostic has already surfaced the issue elsewhere.
| deno_package_json::PackageJsonDepValue::Workspace(_) => { | ||
| // ignore workspace deps | ||
| } | ||
| deno_package_json::PackageJsonDepValue::Catalog => { |
There was a problem hiding this comment.
Reinforcing @lunadogbot's #3: this Catalog branch checks catalog membership but doesn't validate the version_req:
PackageJsonDepValue::Catalog => {
if workspace.catalog().contains_key(k.as_str()) {
normal_deps.insert(k.to_string());
}
}If workspace.catalog() has "@foo/bar": "not-a-version", this happily adds @foo/bar to the install set. Resolution downstream silently fails with the misleading "not found in catalog" message (per the inline above on libs/resolver/lib.rs).
Gate on a successful parse here, or — preferred — surface bad version_reqs as WorkspaceDiagnostic at load time (which would also fix the lockfile.rs silent-drop). That way the install set only contains catalog entries with parseable versions, and the user sees one diagnostic at config-load time rather than three different downstream failures.
Same pattern at line 108 below (the dev_dependencies arm) — same fix.
|
Update: now that I have push access I've moved the work directly onto this PR's branch.
CI is running on the merge commit; will monitor and follow up if anything fails. Happy to iterate further on naming/wording of the new diagnostic if @bartlomieju has a preference. |
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
LGTM at 6ef639c. The 95af1e5 follow-up commit cleanly addresses all three concerns from my prior COMMENT (and the matching @lunadogbot points):
- Named catalog rejection at parse time —
package_json/lib.rs:199–214now matchesvalue.is_empty() || value == "default"for thecatalogscheme; any other suffix producesUnsupportedNamedCatalog { name }with a clear error string mentioning the rejected name. Test fixtures cover bothcatalog:default(accepted) andcatalog:react18(rejected). pnpm/Bun users who try named-catalog semantics will see a typed error rather than the previous silent coercion. - Installer gate via resolve_catalog_dep —
cli/tools/installer/local.rs:84–94, 110–114now useworkspace.resolve_catalog_dep(k.as_str()).is_some()instead of plaincontains_key(). Inline comment explicitly calls out the failure mode that fix prevents ("a typo in the catalog won't silently add the dep here only to fail later with a misleading 'not found in catalog' error"). - Config-load diagnostic —
libs/config/workspace/mod.rs:185–189, 1249–1262addsWorkspaceDiagnosticKind::InvalidCatalogVersionReq { name, version_req }and emits it during workspace validation whenVersionReq::parse_from_npm(version_req).is_err(). The user sees a single early diagnostic naming the offending entry rather than chasing a downstream resolution error.
The new spec test tests/specs/install/catalog_named_unsupported/ exercises path #1 end-to-end. CI 133/136 success, 2 skip (canary + cla pending), no failures.
This is a new public protocol (catalog: in package.json plus a new catalog root key in deno.json) so it's escalation-class — but you're the author and the substance walks clean now, so flipping to APPROVE on the substance contract. The protocol-design call (catalog: scheme + workspace-root-only + named-catalog rejection) is your decision to make as maintainer.
- Change Catalog from unit variant to Catalog(String) carrying the
catalog name ("default" for catalog:/catalog:default, otherwise
the actual name like "react18")
- Add "catalogs" (plural) field to both deno.json and package.json
for named catalog groups
- When root package.json has catalog/catalogs, use those exclusively
(no merging with deno.json)
- Add root-only validation for catalog/catalogs in both config types
- Update all resolution paths to pass catalog name through
- Add spec tests for named catalogs, package.json catalogs, and
package.json precedence over deno.json
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
Heads-up: this is a substantial scope expansion beyond what I'd APPROVED at 6ef639c (default-only catalogs with named-catalog rejection). The new commit f069144 reverses the rejection decision and adds the named-catalogs + catalogs plural surface that was deliberately left out of the original PR.
What changed in f069144
PackageJsonDepValue::Catalog→Catalog(String)atlibs/package_json/lib.rs:150. The variant now carries the catalog name ("default"forcatalog:andcatalog:default, otherwise the literal name like"react18").UnsupportedNamedCatalogerror variant removed.catalog:react18is now accepted, where my prior review explicitly endorsed rejecting it.package.jsongainscatalog/catalogsfields (libs/package_json/lib.rs:304–307). Previously catalogs lived only in workspace-rootdeno.json; nowpackage.jsonis also a definition site, with root-level precedence rules ("When root package.json has catalog/catalogs, use those exclusively — no merging with deno.json" per the commit message).deno.jsongains acatalogsplural field for named-catalog groups.- All resolution paths updated to thread catalog name through:
cli/args/mod.rs,cli/lib/standalone/binary.rs,cli/lsp/resolver.rs,cli/rt/run.rs,cli/tools/installer/local.rs,cli/tools/publish/unfurl.rs,libs/config/workspace/mod.rs,libs/resolver/{lib,lockfile,workspace}.rs, etc. - New spec tests:
catalog_named/,catalog_pkg_json/,catalog_pkg_json_precedence/. Existingcatalog_named_unsupported/updated (the rejection test now expects acceptance).
Why my prior APPROVE no longer applies
My pullrequestreview-4212820283 approved the narrow feature: workspace-root deno.json with default-only catalog. The escalation reasoning in that approval was based on "new public protocol" and the substance contract was that named catalogs would error early to leave room for design discussion. This commit silently flips that contract.
The expanded surface is substantively reasonable — pnpm/Bun do support named catalogs, and matching them is the right end-state — but it's a different review than what landed previously, and the change deserves its own pass.
Concerns worth maintainer eye
- package.json + deno.json catalog precedence: "When root package.json has catalog/catalogs, use those exclusively (no merging with deno.json)." That's a sharp-edged choice — if a workspace has both, the deno.json catalog is silently ignored. Worth a
WorkspaceDiagnosticwarning ("both package.json and deno.json define catalogs; package.json takes precedence and deno.json is ignored") so users don't get caught out. catalog_pkg_json_precedence/spec test verifies the precedence direction but it's the only check — worth pinning the diagnostic shape (or absence-of-diagnostic) explicitly.- Migration story: existing users who adopted
catalog:in deno.json (the original PR's surface) now need to also know about thecatalogsplural form, named catalogs, and the package.json/deno.json precedence rule. Worth a docs PR alongside this one.
CI
54 success / 22 in-progress / 1 cla pending at f069144. No failures yet but build/test_unit/test_node_compat shards are still landing — substance is fine but the precedence + named-catalog edge cases deserve full CI before merge.
Not blocking — but lifting my prior APPROVE back to COMMENT since this is materially a different feature than the one I reviewed. Worth a fresh org-member maintainer eye on the package.json/deno.json precedence + named-catalog design.
|
@bartlomieju I tried this commit with opencode repo https://github.com/anomalyco/opencode it shows this warning Is that expected ? |
|
This is a real bug — The override parser ( The Fix: #33799 |
The npm overrides parser handles `npm:`, `jsr:`, `$ref`, and plain version strings, but was missing support for `catalog:` references. When a `package.json` used `"catalog:"` or `"catalog:<name>"` in its `overrides` field, the parser tried to interpret it as a semver range and emitted: ``` Warning failed to parse npm overrides: Failed to parse override value "catalog:" for key "@types/bun": Invalid version requirement ``` This adds a `catalog:` branch to `parse_override_string` that resolves the entry from workspace catalogs, then parses the resolved version string. Both `catalog:` (default catalog) and `catalog:<name>` (named catalog) are supported. Reported in #32947 (comment)
The npm overrides parser handles `npm:`, `jsr:`, `$ref`, and plain version strings, but was missing support for `catalog:` references. When a `package.json` used `"catalog:"` or `"catalog:<name>"` in its `overrides` field, the parser tried to interpret it as a semver range and emitted: ``` Warning failed to parse npm overrides: Failed to parse override value "catalog:" for key "@types/bun": Invalid version requirement ``` This adds a `catalog:` branch to `parse_override_string` that resolves the entry from workspace catalogs, then parses the resolved version string. Both `catalog:` (default catalog) and `catalog:<name>` (named catalog) are supported. Reported in denoland#32947 (comment)
4 participants
Summary
Adds
catalog:protocol support forpackage.jsondependencies in workspace members, enabling centralized
version management in monorepos. This matches the catalog
feature available in pnpm, Bun, and Yarn.
to version requirement)
"pkg": "catalog:"or"pkg": "catalog:<name>"in theirpackage.json"catalog"(singular) and"catalogs"(plural) fieldsdeno.jsonorroot
package.json; ifpackage.jsonhas any catalogconfig it takes exclusive precedence (no merging)
resolution, lockfile, publish, and all other code paths
Example: default catalog in
deno.jsonRoot
deno.json:{ "workspace": ["./packages/a", "./packages/b"], "catalog": { "react": "^18.3.0", "react-dom": "^18.3.0", "chalk": "^5.3.0" } }Member
packages/a/package.json:{ "dependencies": { "react": "catalog:", "react-dom": "catalog:" } }Example: named catalogs in
deno.jsonRoot
deno.json:{ "workspace": ["./packages/a", "./packages/b"], "catalog": { "chalk": "^5.3.0" }, "catalogs": { "react18": { "react": "^18.3.0", "react-dom": "^18.3.0" }, "react19": { "react": "^19.0.0", "react-dom": "^19.0.0" } } }Member
packages/a/package.json:{ "dependencies": { "chalk": "catalog:", "react": "catalog:react19", "react-dom": "catalog:react19" } }Member
packages/b/package.json:{ "dependencies": { "chalk": "catalog:", "react": "catalog:react18", "react-dom": "catalog:react18" } }Example: catalog in root
package.jsonRoot
package.json:{ "catalog": { "react": "^19.0.0" }, "catalogs": { "testing": { "jest": "^30.0.0" } } }Root
deno.json:{ "workspace": ["./packages/a"] // no catalog here; package.json's catalog is used }When both
deno.jsonandpackage.jsondefine catalogs,package.jsonwins entirely (no merging between them).Closes #30403
Test plan
Catalog(String)variant onPackageJsonDepValuewith parsing for
catalog:,catalog:default,catalog:<name>catalog/catalogsfields onConfigFileJson,PackageJson,Workspace,WorkspaceResolver, andJSON schema
Catalogin all resolution paths: install,module resolution, lockfile, publish/unfurl, BYONM,
runtime, info, args, local installer, outdated/audit
catalog/catalogsare root-only options(diagnostic emitted if in workspace members)
catalog_deps,catalog_named,catalog_pkg_json,catalog_pkg_json_precedence,catalog_deps_missing,catalog_named_unsupportedcargo test -p deno_package_json)cargo checkclean./tools/lint.jsclean./tools/format.jsclean