Skip to content

feat(compile): support WebCache API/LocalStorage API#33775

Closed
CertainLach wants to merge 2 commits into
denoland:mainfrom
CertainLach:push-wpnroysqptmo
Closed

feat(compile): support WebCache API/LocalStorage API#33775
CertainLach wants to merge 2 commits into
denoland:mainfrom
CertainLach:push-wpnroysqptmo

Conversation

@CertainLach

Copy link
Copy Markdown
Contributor

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 compile execution, 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_intel

deno compile --location is required to be specified for those features to work

CacheStorage 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)

@CertainLach

Copy link
Copy Markdown
Contributor Author

Also
Fixes: #10693

@fibibot fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. storage_key_resolver: built from metadata.location via StorageKeyResolver::from_flag when present, else empty(). This is what gates both localStorage and caches — both APIs derive their per-origin namespacing from the same resolver.
  2. origin_data_folder_path: now Some(deno_dir.compile_origin_data_folder_path()) (was None), pointing at <deno_dir>/origin_data/. New helper added cleanly to DenoDir next to the existing location_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)

  1. Origin-hash directory name vs human-readable. Worth a maintainer call. Origin hash matches what deno run does today and keeps cross-platform consistency (no path-escaping for arbitrary URL chars). Human-readable would help debugging but breaks if --location includes unusual characters. I'd lean origin-hash for parity unless there's a strong reason to diverge in compile.
  2. origin_data vs location_data. Following the TODO(@crowlKats): change to origin_data for 2.0 is 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 existing location_data_folder_path() helper is unchanged here, so deno run users aren't affected.)
  3. CacheStorage persistence. Probably fine as non-persisted for an initial cut — Node's fs/promises exists 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

  • cd0e4b8 only 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, so deno run's storage behavior is unaffected.
  • --location is the documented surface for opting into per-origin storage in deno 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.

@bartlomieju

Copy link
Copy Markdown
Member

Thanks for this PR! The approach looks correct — the wiring is minimal and properly maintains the invariant at cli/lib/worker.rs:636.

Two requests:

  1. Please rebase onto main — there have been a few recent changes in this area.

  2. Test plan: Could you come up with a spec test we can land in a follow-up? Something like:

    tests/specs/compile/storage_apis/
      __test__.jsonc
      main.ts
    

    Where main.ts exercises localStorage.setItem/getItem (and optionally caches.open) with --location=https://example.com, and the test asserts it doesn't throw. We can figure out the denort logistics separately, but having the test shape ready would help move this forward.

Minor nit: the DenoDir construction happens unconditionally even when metadata.location is None. Consider moving it inside the .map() branch so it's only constructed when actually needed.

@lunadogbot lunadogbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. deno_cache_dir::resolve_deno_dir(...) now runs before LibMainWorkerOptions is built, even when metadata.location is None. That makes every compiled binary depend on resolving DENO_DIR/home/cache at startup, although storage still stays disabled without --location. Build origin_data_folder_path in the same metadata.location.as_ref().map(...) branch as storage_key_resolver so binaries that do not opt into origin storage keep the old startup path.

  2. This adds runtime-visible deno compile behavior without a regression test. Please add a spec that compiles with --location and exercises localStorage at minimum; without that, the origin_storage_dir/storage-key wiring can regress silently.

CI has not run beyond lint title, so this is not approvable yet either.

@fibibot fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. deno_cache_dir::resolve_deno_dir(...) now runs before LibMainWorkerOptions is built, even when metadata.location is None. That makes every compiled binary resolve DENO_DIR/home/cache at startup although storage remains disabled without --location; build origin_data_folder_path inside the same metadata.location.as_ref().map(...) branch as storage_key_resolver so non-storage binaries keep the old startup path.

  2. This adds runtime-visible deno compile behavior without a regression test. Please add a spec that compiles with --location and exercises localStorage at minimum; otherwise the origin_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>
@CertainLach

Copy link
Copy Markdown
Contributor Author

@bartlomieju rebased, added a test

Comment thread cli/rt/run.rs
let (storage_key_resolver, origin_data_folder_path) =
match metadata.location.as_ref() {
Some(location) => {
let deno_dir = deno_resolver::cache::DenoDir::new(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 fibibot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. console.log(await res.text()); fails type checking because cache.match() returns Response | undefined; every failing test specs (1/2) shard reports TS18048 here. Make the test prove the cache hit first, for example if (!res) throw new Error("cache miss");, then read res.text().

@lunadogbot lunadogbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. console.log(await res.text()); in tests/specs/compile/storage_apis/main.ts dereferences Response | undefined; cache.match() can miss, and the test specs (1/2) shards fail on this file. Add a guard like if (!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>
@bartlomieju

Copy link
Copy Markdown
Member

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 localStorage, caches, and default Deno.openKv() in compiled binaries through a single mechanism. It also addresses the open question you raised in this PR's description about a human-readable storage directory: rather than an origin hash under DENO_DIR, a compiled binary is treated as a standalone app and its storage lives under the platform's app data directory keyed by a new --app-name flag (defaulting to the output file name). Your localStorage/caches test coverage is folded into that PR. Closing in favor of #34618.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

deno_cache panics when using compile

4 participants