feat(compile): support WebCache API/LocalStorage API#33775
Conversation
|
Also |
fibibot
left a comment
There was a problem hiding this comment.
Thanks for the AI-disclosure ("No LLM was used") and the linked context to #18889 — the diagnosis is right that the underlying issue from that closed thread (storage APIs not enabled in compiled binaries) hasn't actually been fixed.
What this PR does
The wiring in cli/rt/run.rs:1043–1107 is minimal and correct:
storage_key_resolver: built frommetadata.locationviaStorageKeyResolver::from_flagwhen present, elseempty(). This is what gates bothlocalStorageandcaches— both APIs derive their per-origin namespacing from the same resolver.origin_data_folder_path: nowSome(deno_dir.compile_origin_data_folder_path())(wasNone), pointing at<deno_dir>/origin_data/. New helper added cleanly toDenoDirnext to the existinglocation_data_folder_path().
The caches API's temp-dir pathing ($TMPDIR/deno_cache/$HASHED_ORIGIN/) wires up automatically once the storage key resolver is non-empty, since the existing default for cache_storage_dir works without a compile-specific override. So the title's "WebCache API" claim does hold, even though the diff only obviously touches LocalStorage's persistent path — both surfaces light up via the single resolver change.
The --location requirement is correct: StorageKeyResolver::from_flag returns a meaningful resolver only when location is set, so without that flag both APIs continue to throw, matching standard deno run behavior.
Discussion points (from your PR body)
- Origin-hash directory name vs human-readable. Worth a maintainer call. Origin hash matches what
deno rundoes today and keeps cross-platform consistency (no path-escaping for arbitrary URL chars). Human-readable would help debugging but breaks if--locationincludes unusual characters. I'd lean origin-hash for parity unless there's a strong reason to diverge in compile. origin_datavslocation_data. Following theTODO(@crowlKats): change to origin_data for 2.0is the right call — compile is a clean slate with no backward-compat constraint, so adopting the planned-future name now avoids a later migration. (And the existinglocation_data_folder_path()helper is unchanged here, sodeno runusers aren't affected.)- CacheStorage persistence. Probably fine as non-persisted for an initial cut — Node's
fs/promisesexists for users who want explicit persistence, and quota management is genuinely tricky. Worth flagging this in user-facing docs once it lands so users don't expect cache survival across compile-binary restarts.
Risk surface
cd0e4b8only touches two files (run.rs + deno_dir.rs); no public API/permission/runtime-init changes outside the compile path. The non-compile worker setup (cli/main.rs etc.) is unchanged, sodeno run's storage behavior is unaffected.--locationis the documented surface for opting into per-origin storage indeno compile, so users who don't set it see the same "throws on access" behavior they had before this PR. No silent default-on activation.
Test honesty
Your observation about cross_compile_intel_mac/denort_intel is right — the existing pattern is checking a denort binary into the test fixtures dir (uncomfortable but works). For this PR, a compile-then-run-binary spec test that exercises localStorage.setItem after --location=https://example.com would be the test shape, but yes — it requires a denort binary that's built from this branch. Worth coordinating with @bartlomieju on whether the test can be skipped / gated until the cross-compile pipeline lands.
CI / process
Only lint title has run so far (1 success, license/cla pending). Substance is small enough that I'd expect green once the build matrix kicks off. cc @bartlomieju on the design questions in your PR body — this is a public-facing deno compile feature addition, which is exactly the maintainer-eye case for new compile-binary surface area, regardless of CI state.
|
Thanks for this PR! The approach looks correct — the wiring is minimal and properly maintains the invariant at Two requests:
Minor nit: the |
lunadogbot
left a comment
There was a problem hiding this comment.
-
deno_cache_dir::resolve_deno_dir(...)now runs beforeLibMainWorkerOptionsis built, even whenmetadata.locationisNone. That makes every compiled binary depend on resolvingDENO_DIR/home/cache at startup, although storage still stays disabled without--location. Buildorigin_data_folder_pathin the samemetadata.location.as_ref().map(...)branch asstorage_key_resolverso binaries that do not opt into origin storage keep the old startup path. -
This adds runtime-visible
deno compilebehavior without a regression test. Please add a spec that compiles with--locationand exerciseslocalStorageat minimum; without that, theorigin_storage_dir/storage-key wiring can regress silently.
CI has not run beyond lint title, so this is not approvable yet either.
fibibot
left a comment
There was a problem hiding this comment.
-
deno_cache_dir::resolve_deno_dir(...)now runs beforeLibMainWorkerOptionsis built, even whenmetadata.locationisNone. That makes every compiled binary resolveDENO_DIR/home/cache at startup although storage remains disabled without--location; buildorigin_data_folder_pathinside the samemetadata.location.as_ref().map(...)branch asstorage_key_resolverso non-storage binaries keep the old startup path. -
This adds runtime-visible
deno compilebehavior without a regression test. Please add a spec that compiles with--locationand exerciseslocalStorageat minimum; otherwise theorigin_storage_dir/ storage-key wiring can regress silently.
CI has not run beyond lint title and CLA on this head, so this is not approvable yet either.
Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
cd0e4b8 to
fbfc9ff
Compare
|
@bartlomieju rebased, added a test |
| let (storage_key_resolver, origin_data_folder_path) = | ||
| match metadata.location.as_ref() { | ||
| Some(location) => { | ||
| let deno_dir = deno_resolver::cache::DenoDir::new( |
There was a problem hiding this comment.
Btw, is there a policy on style guide or idk? Too many things are used by absolute paths and it feels sloppy
I did not add anything single-use here, but idk if I should, my finger itches to perform "replace with use" code action.
fibibot
left a comment
There was a problem hiding this comment.
This still needs changes before merge. The previous startup-path blocker is fixed by constructing DenoDir only inside the metadata.location branch, but the new regression test currently fails CI.
console.log(await res.text());fails type checking becausecache.match()returnsResponse | undefined; every failingtest specs (1/2)shard reports TS18048 here. Make the test prove the cache hit first, for exampleif (!res) throw new Error("cache miss");, then readres.text().
lunadogbot
left a comment
There was a problem hiding this comment.
The earlier startup-path blocker is fixed: DenoDir is now built only in the metadata.location branch. The new regression spec still fails to type-check.
console.log(await res.text());intests/specs/compile/storage_apis/main.tsdereferencesResponse | undefined;cache.match()can miss, and thetest specs (1/2)shards fail on this file. Add a guard likeif (!res) throw new Error("cache miss");before reading the body so the test proves the cache entry exists.
Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
fbfc9ff to
31db2a4
Compare
|
Thanks again for the original diagnosis here and on #18889/#10693 — the underlying problem (storage APIs not enabled in compiled binaries) is real and your analysis was spot on. Superseded by #34618, which enables |
Fixes: #18889 (I mean, it is closed, but the underlying issue was not solved yet - cache was not enabled for compiled binaries)
Note that for testing this it is not enough to build deno from this branch, denort is downloaded from the internet during
deno compileexecution, and it gets weird. I'm also not how automate testing of this thing, the only test overriding denort seems to store denort binary in the repo (?): https://github.com/denoland/deno/blob/2d7722227068aff1a4dbfb53c85c6dff93b5ad18/tests/specs/compile/cross_compile_intel_mac/denort_inteldeno compile
--locationis required to be specified for those features to workCacheStorage is located at the same path as standard deno:
$TMPDIR/deno_cache/$HASHED_ORIGIN/It is not persisted, same as in deno, because of comments regarding storage quota management (should we care about that for compile?)
LocalStorage is located at
$HOME/.cache/deno/origin_data/$HASHED_ORIGIN/local_storage(Compared to original deno, I respected this comment:
TODO(@crowlKats): change to origin_data for 2.0, as we should care more for compiled binaries, I think? And we don't care about backwards compat here as compile was not supporting LocalStorage before)But I think we should instead have human-readable directory name here instead of origin hash? Should it be derived from location, or it would be better to be specified explicitly?
Initially I have planned to implement cache storage in the same directory as local_storage, but found out a discussion is required here, so I dropped it half way, since I'm not sure about the design, this PR is an invitation to this discussion, I'll implement whatever is decided, as I just would like to have web cache api available directly.
For my current use-cases, I do not care about CacheStorage persistence, the current implementation is good enough for me for long-running backend services.
Cc: @sigmaSd (as a workaround author) @tugrulates (As someone who was also interested in this feature)