feat(outdated): warn about packages skipped due to registry errors#34974
Conversation
Previously, when `deno outdated` could not fetch a package's metadata (e.g. an unreachable or unauthorized private registry), the package was silently dropped from the table, indistinguishable from being up to date. The package info fetch errors are now threaded through instead of being collapsed into `None`, and `deno outdated` prints a warning listing how many packages could not be checked. With `--log-level=debug`, it lists each package, its registry, and the failure reason. Closes #32179
ReviewClean, well-targeted change. The A few things worth addressing:
Non-blocking notes: transient network errors get memoized as hard errors for the resolver's lifetime (matches the prior No correctness concerns — logic is sound and the test uses the discard port for a deterministic failure. The main thing I'd resolve before merge is #1. |
- npm now stores the registry base URL (matching jsr) instead of the per-package metadata URL, and avoids computing get_package_url twice. - Extract collect_skipped/SkippedPackages and surface the skipped-packages warning in the `outdated --update` path too, not just the listing path.
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Review: LGTM ✅
The second commit (2b813c0) resolves all three points from the earlier review:
- Consistent
registry_url— npm now stores the registry base URL vianpmrc.get_registry_url(name)(e.g.http://localhost:9/), matching jsr'sjsr_url().warning_verbose.outupdated accordingly. - Update path warns too —
print_skipped_warningis now called inupdate(), withcollect_skippedrunning before anycontinue. - No double
get_package_url— the error path usesget_registry_url;get_package_urlis computed once insidefetch_package_info.
Other things I verified:
DepKindderivesOrdand hasscheme(), so theBTreeMap<(DepKind, StackString), _>key is valid.- The filter (
deno outdated --update <filter>) is applied upstream inDepManager::from_workspace[_dir], so the skip warning only covers in-scope packages — no noise from filtered-out deps. collect_skippedruns before thelet Some(resolved) = … else { continue }guard, so skips still surface when the current version can't resolve.- Errors from the earlier
req_to_nvfetch are cached ininfo_by_nameand reused by the latest-version computation (one fetch, error preserved). - The test uses the discard port (
localhost:9) for a deterministic, fast connection-refused failure.
Non-blocking: transient network errors get memoized as hard errors for the resolver's lifetime, but the resolver is per-invocation so it's not a regression; and reason: String flattens the error early, which is fine for a diagnostic-only field.
Clean, well-targeted, good test coverage. Just needs CI to go green before merge.
1 participant
When
deno outdatedcould not fetch a package's metadata, for examplebecause it lives on an unreachable or unauthorized private registry, the
package was silently dropped from the table. There was no way to tell that
apart from the package being up to date, which is confusing for enterprise
users who depend on private registries.
The package metadata fetch errors were collapsed into
Nonebefore theyreached the caller, so the skip site had nothing to report. This threads
the failure reason (and the registry it came from) through the npm and jsr
fetch resolvers, and has
deno outdatedprint a warning telling the userhow many packages could not be checked. Running with
--log-level=debugadditionally lists each affected package, its registry, and the underlying
reason.
Closes #32179