Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Broad provider, storage, and credential-persistence changes warrant final human review.
Review effort: Lite
Findings: None
What changed in this PR
Adds configurable asynchronous cache adapters and browser persistence across provider components.
Changes:
- Adds memory, IndexedDB, Web Storage, and TTL caches.
- Injects caches into issuer, discovery, client, and DPoP providers.
- Adds browser defaults, persistence tests, smoke tests, and CI coverage.
| File | Description |
|---|---|
test/providers.test.js |
Tests provider cache injection and behavior. |
test/dpop-cache.test.js |
Tests credential persistence and refresh rotation. |
test/cache.test.js |
Tests adapters, failures, namespaces, and TTLs. |
test/browser-cache.html |
Tests browser persistence and isolation. |
src/WebStorageCache.ts |
Implements codec-based Web Storage caching. |
src/mod.ts |
Exports cache APIs and adapters. |
src/MemoryCache.ts |
Implements in-memory caching. |
src/IndexedDbCache.ts |
Implements IndexedDB persistence. |
src/ExpiringCache.ts |
Adds absolute-TTL behavior. |
src/DPoPTokenProvider.ts |
Adds injectable credential caching and rotation handling. |
src/createBrowserCaches.ts |
Provides browser cache defaults. |
src/CachingIssuerProvider.ts |
Adds injectable issuer caching. |
src/CachingClientProvider.ts |
Adds injectable client caching. |
src/CachingAuthorizationServerProvider.ts |
Adds injectable discovery caching. |
src/Cache.ts |
Defines the asynchronous cache contract. |
README.md |
Documents cache configuration and browser storage policy. |
package.json |
Adds test tooling and dependencies. |
.github/workflows/ci.yml |
Runs the expanded test suite. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@langsamu note that I have taken a brief look at this PR. Overall I think it is good. I was curious as to whether there is a good library out there that we can use for the generic caching interface. The recommendation from Astra was to use idb-keyval to cleanup the The full response can be viewed here. EDIT: It turns out you can synthesise a library with exactly the functionality we want first shot with Astra. See it available here. Note the interface uses |
On
main,CachingIssuerProvider,CachingAuthorizationServerProvider,CachingClientProviderandDPoPTokenProvidereach keep a privateMap. This PR replaces all four with a private field typed against a small internal asyncCache<T>interface (getItem,setItem,removeItem). The interface is shaped like KeyValueKit'sKeyValueStore, but KeyValueKit is not added as a dependency. The single implementation,MemoryCache<T>, is backed by aMapand returns the stored object itself. A miss returnsnull, and the value type excludesnullandundefined; this is a type constraint only, not checked at runtime. Every cache operation is now awaited.Nothing is exposed to callers. Constructors and
src/mod.tsexports are unchanged. Cache keys, per-instance caches, the returned objects (including issuerURLs), DPoP refresh and removal order, and the Web Lock scope all behave as onmain. Related #45: caller injection and persistent storage are not part of this change. Storage-wide cache keys remain separate in #46.Validation
npm run build(tsc) succeeds..d.tsfiles are byte-identical to a build ofmain, anddist/mod.jsis identical too.node:testchecks (not committed) ran against the built output with stubbed endpoints. They cover the memory cache, provider hits, misses and retries, and DPoP caching, locking and refresh. All pass on this branch, and the 12 provider checks also pass onmain.Original user request (verbatim)
https://github.com/solid-contrib/reactive-authentication The link provided is to a GitHub repository for a library called Reactive Authentication. The design philosophy behind this library is to reactively upgrade fetch requests when an unauthorized response is returned, and it has been first built for Solid applications. The library, as it stands, is starting to implement caching so that, say issuer metadata, refresh tokens, and depop tokens can be cached and reused to upgrade requests. I would like you to help improve the caching functionality. Specifically, all of the caching at present is done in memory, and the caching is per URL rather than per storage in the context of Solid or LWS. I would like to open an issue, if there is not already one, and I would like you to check if there is already an issue, for both extending in-memory caching to other caching types, including in the browser, using appropriate storages. So the way that I want caches designed is to be configurable. So there's a generic cache interface, and then different components of this library can be instantiated with those caching interfaces. In particular, there's going to be a good browser default, which I imagine you can recommend based on your knowledge of the browser APIs, the credential APIs, the local storage APIs, and which of those APIs are going to be appropriate for caching different types of data that are cached in the library. And then I would like you to open an issue for improving the keying mechanism for caches so that it doesn't always cache just on the request URL, instead it caches to the appropriate context, i.e. storage for linked web storage and Solid. For that storage-wide context, open just an issue at the moment and not a PR. I would like you to look at the existing open PRs and see if they address these issues. Older PRs would have been opened by a less capable LLM to yourself, so you are going to need to start from scratch. But I would like you to reference any PRs that you're superseding and close them as you reference them. Finally, I would like you to include in a fold-out part of the PR description, so it's not there by default, the prompt that I have just given you, so the full context of this conversation is available to the maintainer of the library.