-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
feat: hydratable and friends
#16960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: hydratable and friends
#16960
Conversation
|
|
Very interesting approach |
|
|
|
||
| /** @typedef {{ count: number, item: any }} Entry */ | ||
| /** @type {Map<string, CacheEntry>} */ | ||
| const client_cache = new Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the intent here is for libraries to provide a prefix if they're going to use this API, should this be a two-tiered cache? Right now, if a library (like SvelteKit) wants to do something to everything it has in the cache, it has to iterate over all of the map entries, skipping the ones that don't start with its prefix, and refresh the things that do start with its prefix.
Maybe what we should do is make this two-tiered, where, if you don't provide a prefix, everything gets put into client_cache.get(''), but if you do, you get a namespaced cache. So in the case of SvelteKit, we'd end up with client_cache.get('@sveltejs/kit/remote'), which can be operated on as its own entity.
From an API perspective, you can provide a prefix as part of a third options argument to cache. Then, if you use CacheObserver, providing a prefix will automatically scope it to your cache.
The downside would be if you truly wanted to operate on the entire cache, which would be more complicated...
| * @returns {Resource<TReturn>} | ||
| */ | ||
| export function fetcher(url, init) { | ||
| const key = `svelte/fetcher/${typeof url === 'string' ? url : url.toString()}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given how simple this is I'm quite tempted to say "nah, this can be an example in the docs" instead of shipping it as a core API...
| static #hydratable_block(serialized) { | ||
| let entries = []; | ||
| for (const [k, v] of serialized) { | ||
| entries.push(`["${k}",${v}]`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been struggling with what the correct thing to do here is. Is it to JSON.stringify the key, and leave the value to whatever value.toString results in? When a user provides encode, does that function have to result in a string? Or does it have to result in something that can be toString-ed? Or something else entirely?
| /** | ||
| * @template T | ||
| * @implements {ReadonlyMap<string, T>} */ | ||
| export class BaseCacheObserver { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here -- should this actually be a two-tier cache, where the key provided here gives you access to a map specific to that key?
| if (!key.startsWith(this.#prefix)) continue; | ||
| yield /** @type {[string, T]} */ ([key, entry.item]); | ||
| } | ||
| return undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is required for the implementation to succeed in typescript 🤔
| export type Transport<T> = | ||
| | { | ||
| encode: Encode<T>; | ||
| decode?: undefined; | ||
| } | ||
| | { | ||
| encode?: undefined; | ||
| decode: Decode<T>; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this rather unique way of describing this type will force people to use browser ? { decode: () => {} } : { encode: () => {} } so that encode/decode can be properly treeshaken in the correct environments
| export async function fetch_json(url, init) { | ||
| const response = await fetch(url, init); | ||
| if (!response.ok) { | ||
| throw new Error(`TODO error: Fetch error: ${response.status} ${response.statusText}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO

This is a draft, it's not ready, leave me alone
Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint