fix(cache): allow no-remote to read vendored modules#34827
fix(cache): allow no-remote to read vendored modules#34827puneetdixit200 wants to merge 1 commit into
Conversation
bartlomieju
left a comment
There was a problem hiding this comment.
Thanks for tackling this, and for the clear repro and the AI disclosure. The vendor case the issue describes is real and this does fix it. But the implementation broadens --no-remote further than the issue intends — it changes the flag's behavior for all projects, not just vendored manifests. I think it needs to be scoped down (or get an explicit maintainer call). Details inline; I verified the behavior change locally.
| let cache_setting = | ||
| options.maybe_cache_setting.unwrap_or(&self.cache_setting); | ||
| if !self.allow_remote { | ||
| if self.should_use_cache(url, cache_setting) |
There was a problem hiding this comment.
This reads from self.http_cache, which is not vendor-only:
- With
"vendor": true,http_cacheis aLocalHttpCachebuilt with the global cache as a fallback (GlobalToLocalCopy::Allow,libs/resolver/factory.rs:510). - Without vendor,
http_cacheis the globalDENO_DIRcache directly (factory.rs:518).
So this makes --no-remote serve http/https from the global cache in all cases, not just vendored manifests. That changes long-standing --no-remote semantics: a remote import that merely happens to be in the global cache now loads, where before it errored — effectively turning --no-remote into --cached-only for cached modules, and dropping the strict "no remote modules" guarantee that distinguishes the two flags.
I confirmed this on a non-vendored project with a globally-cached jsr:@std/fmt:
- today:
error: JSR package manifest for '@std/fmt' failed to load. A remote specifier was requested ... but --no-remote is specified. - with this change it would instead resolve from the global cache.
The issue is specifically about vendored jsr/npm manifests (it even notes --cached-only already covers the offline case), so suggest scoping this cache read to the vendor/local cache only — e.g. only take this path when self.http_cache is a Local (vendor) cache — so non-vendored globally-cached modules still error under --no-remote.
cc @bartlomieju — this is a --no-remote semantics change; worth a maintainer call on whether serving from the global cache is acceptable or it should be vendor-scoped.
| ], | ||
| "output": "" | ||
| }, { | ||
| "args": "run --no-remote main.ts", |
There was a problem hiding this comment.
nit: this spec only covers the vendor case (DENO_DIR wiped). Once the scope is decided, please also pin the non-vendor boundary — a globally-cached jsr:/https: import run with --no-remote and no vendor — so the intended behavior is locked in and can't silently regress.
|
Deepak kudi seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Fixes #34793.
Summary:
NoRemoteDENO_DIRand runs the vendored module with--no-remoteAI assistance disclosure: I used AI tooling while developing this patch and reviewed the final code and tests.
Tests:
cargo test -p specs_tests --test specs vendor_no_remotetarget/debug/deps/specs-4c25e2a61e04fe22 vendor_no_remotePATH="$PWD/target/debug:$PATH" deno run -A --no-config npm:dprint@0.47.2 check --config=.dprint.json libs/cache_dir/file_fetcher/mod.rs tests/specs/jsr/vendor_no_remote/__test__.jsonc tests/specs/jsr/vendor_no_remote/deno.json tests/specs/jsr/vendor_no_remote/main.tsPATH="$PWD/target/debug:$PATH" deno run -A --config=tests/config/deno.json tools/format.js --check