-
Notifications
You must be signed in to change notification settings - Fork 529
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
implement CacheStorage? #2071
Comments
I don't have time to do it myself but I'd be happy to see something like this added. |
I'll look into it. |
I believe we would need sqlite or some alternative to implement this correctly, which makes it a non-starter: "Furthermore, because CacheStorage requires file-system access...". Deno also uses sqlite for this. |
Oh, didn't know deno had cache storage, that is cool :) |
If node was to implement some form of sqlite database this would be actionable, but we can't really do anything better than an in-memory cache (which is worse than a userland alternative). |
RocksDB with the BlobDB variant would be a perfect fit IMHO |
@mcollina Any thoughts? Would vendoring a db into node core even be an option? |
I think it would be good if the nodes http-loader could utilize fetch + CacheStorage. |
I think it would be useful, but better as another library. Nothing requires fetch internals afaict so it should be possible. Adding a postinstall script and a million build dependencies doesn't seem like a good idea for undici. |
What if you want to use: |
I was talking about the CacheStorage implementation itself, not really integrating it into fetch, which might need fetch internals. There are a number of fetch options that are either ignored or have no effect, so setting |
Here's what I came up with before I got tired of working on it: https://github.com/KhafraDev/node-cachestorage It implements CacheStorage and Cache. None of Cache is implemented other than matching so you can't really test it out. If node ever got a database of some sort I'd probably copy this implementation into undici so feel free to contribute 😄. |
To be honest, I don't think a naive implementation needs a database for this. We could just build it on top of a temporary folder and hashed URLs. |
yeah, but then it's a worse version of something that can be done elsewhere. It also adds a lot more complexity to an already complicated (badly written) spec. |
Having a IndexedDB would be something useful. and other databases are not so good at saving javascript objects that are structural cloneable. it goes very much hand in hand with the I could definitely see me building a CacheStorage on top of IndexedDB. |
@benjamingr does implementing a db make us more web compliant? |
Now when i think about it... @mcollina have a good argument about using the file system. and doing things such as: const responseClone = response.clone()
cache.put(request, responseClone)
return response lets you operate on it much earlier in parallel then if you had to put everything in a database and wait for it to be copied over with a single ArrayBuffer or something The cache storage pretty much lets you iterate over all Request/Responses and sees them. So a file based storage would not actually be that bad if it's stream/memory friendlier |
I mentioned it previously but the CacheStorage/Cache spec is not written as concisely/cleanly as the fetch spec. It's actually much worse than the websocket spec, which is impressive. None of the steps are clear and it feels like steps are skipped over/not implemented (look at Cache.addAll, the spec doesn't mention caching the responses but it expects you to). Adding another layer of complexity here makes it even harder to implement. Referencing Deno: their implementation is massively outdated and doesn't include many methods, either on Cache or CacheStorage. They're missing Not to mention that the only way to open file-backed blobs is through fs.openAsBlob which was added in v19.8.0 and is still experimental. However if you want to implement it (could be a |
I think using e.g. the chromium implementation is the best spec 🤷♂️ |
maybe it dose not necessary have to be just a disc- based blob. openAsBlob would take a long time until everybody could actually start using it... Response / fetch just probably see blobs in the init and convert the body to streams anyway? |
However we also need to cache the body (and request headers and method) because each cache match is meant to return a new Response. I took a look at the chromium implementation and it looks like each cache entry creates 3 temporary files (1 for the body, 1 for headers, and 1 for other request stuff). But since it's in c++ it's kinda hard to translate that to js. |
the Request body isn't saved, right? that's how i remembered it... only the request header & request url is saved? // 1.json
{
url: "https://example.com/cat.png"
request: {
headers,
url
},
response: {
headers,
status,
statusText,
url: response.url // the final response url after redirects
},
}
|
Yep, request body doesn't need to be saved, I was referring to the response body there. I'm not sure why they use 3 files, maybe I just misread because I can't find the comment again. Their implementation creates a directory for each cache (ie. Some limitations/design details/questions:
|
Not as far as I'm aware but it would make it easier to write code that works both in browsers/node which is the point of web compliance in the first place. |
I was wondering if you would be willing to look into implementing the Cache Storage api
it was something i was looking into in the past... but it got abandoned.
it is pretty much tightly coupled with Request/Response classes
and i would like to use it to avoid having to make extra requests if i have the key (Request) cached locally.
it's also in fact tightly coupled with the
cache
option in Request too.it would be neat if this could just work with some default storage and Request / Responses where saved locally on the disc somewhere.
manual working with caches works to...
(taken from mdn)
maybe
caches.open('node:undici')
could store the default request / responses from fetch when using the cache option?The text was updated successfully, but these errors were encountered: