Skip to content
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

fetch cache #3756

Closed
brandonkal opened this issue Jan 23, 2020 · 19 comments
Closed

fetch cache #3756

brandonkal opened this issue Jan 23, 2020 · 19 comments
Labels
feat new feature (which has been agreed to/accepted)

Comments

@brandonkal
Copy link
Contributor

It would be useful to see Deno use a browser-like cache.

I'd like to see fetch requests make use of standard HTTP-caching. That is, honoring Expires, Cache-Control headers, sends If-Modified-Since, and so on.

@brandonkal
Copy link
Contributor Author

Considering building a solution in TS in the meantime.

@TonyLianLong wouldn't this just be return response or am I missing something here?

deno/cli/js/fetch.ts

Lines 449 to 450 in 63293a9

case "manual":
throw notImplemented();

@ry
Copy link
Member

ry commented Jan 23, 2020

Yes, I agree. It would be ideal it could be done at the rust level....

@ry ry added the feat new feature (which has been agreed to/accepted) label Jan 23, 2020
@kitsonk
Copy link
Contributor

kitsonk commented Jan 23, 2020

Most of the logic and capability is in cli/file_fetcher.rs and should be transparent to the OP_FETCH which is defined in cli/ops/fetch.rs.

@TonyLianLong
Copy link
Contributor

TonyLianLong commented Feb 2, 2020

@brandonkal While implementing the redirection for follow, I noticed that the standard for fetch says the following for manual, source:

Retrieves an opaque-redirect filtered response when a request is met with a redirect so that the redirect can be followed manually.

You probably need to first create a response which is opaque-redirect filtered, and then return.

@brandonkal
Copy link
Contributor Author

That makes sense @TonyLianLong

For my particular use case I ended up deploying a serverless proxy worker that wraps text in a JS module default export, thus using Deno's existing caching mechanism for static imports. So I am no longer working on a TS interim solution.

If someone does start working on this, the best route is to use Rust for the default cache (could function without allow-write/read because Rust is priviledged) but allow the user to provide an alternate Cache instance on the TypeScript side. If a TypeScript implementation was provided, the runtime would naturally require the appropriate permissions.

Refer to make-fetch-happen and in particular #opts-cache-manager

This would mean a sane default cache mechanism implemented in Rust but the flexibility for the user to specify an alternate Cache mechanism (i.e. using Redis) using the standard Cache API.

@seishun
Copy link
Contributor

seishun commented Feb 16, 2020

As alluded to above, a fetch cache could be used for static non-code resources, such as .proto files. But it won't be ideal if it would still require --alow-net, and especially if the cache would expire. Compare that to npm packages, where any resources required by the module are stored in a local directory and can be read directly from the file system.

@brandonkal
Copy link
Contributor Author

brandonkal commented Feb 16, 2020

That's one of the issues with npm packages. They can download anything and put it in the PATH. The cache would expire based on the headers the server specifies. If you don't want expiration you specify that where you serve the file.

Rather than making a fs call in the module, you make a fetch call, and if reaching out to the network can be avoided because of a valid cache, it is.
This appears objectively better than depending on an external package manager to do work beforehand to place an asset in a relative location on disk.

I suppose it may make sense to add another permission flag i.e. --allow-offline-fetch which could be used in place of allow-net if resources are cached.

@MierenManz
Copy link

MierenManz commented Jul 26, 2021

CacheStorage and Cache API's are used for this.
It might be worth implementing those API's for deno to allow fetch to cache responses

Cache: https://developer.mozilla.org/en-US/docs/Web/API/Cache
Specification: https://w3c.github.io/ServiceWorker/#cache-interface

Cache Storage: https://developer.mozilla.org/en-US/docs/Web/API/CacheStorage
Specification: https://w3c.github.io/ServiceWorker/#cachestorage-interface

It may also be a good default for third party modules that need to cache data non persistently.
For example discord libraries or file servers.

@kitsonk
Copy link
Contributor

kitsonk commented Jul 26, 2021

Implementing Cache and CacheStorage is a good idea, but it is a slightly different concern from the "DENO_DIR" cache. We should make our cache more browser like, but the browsing cache in browsers is seperate (and not accessible via an API) from Cache and CacheStorage.

There is #9931 about how the internal cache needs to respect headers and behave more browser-like.

@MierenManz
Copy link

As far as I can tell, Cache and CacheStorage are non-persistent
I only glanced at the spec real quick to see how their interfaces look. But I can check for you if you want me to.

@iuioiua
Copy link
Contributor

iuioiua commented Nov 22, 2022

The Cache API is now implemented. I believe this issue can be closed.

@lino-levan
Copy link
Contributor

+1 on @iuioiua's comment, just linking PR for reference since I had to go out of my way looking for it.

ref: #15829

@JoakimCh
Copy link

The Cache API is now implemented. I believe this issue can be closed.

The Cache API is something completely different in my opinion. So it doesn't seem right to close this issue because the Cache API was implemented.

What the issue author asked was for fetch to automatically cache responses based on e.g. the Cache-Control header (basically to match the spec).

Let's say I keep fetching https://www.googleapis.com/oauth2/v3/certs to keep the public keys updated (whenever I use them). In the browser I know that this response will be cached until not "fresh" anymore (see this documentation), hence I know that I'm not going to be hammering that server with requests.

Using the Cache API I must myself parse the headers to understand how to properly cache the response (if I understand correctly), which would be a lot of work and not really related to this issue in my opinion.

@iuioiua
Copy link
Contributor

iuioiua commented Sep 17, 2023

The Cache API is now implemented. I believe this issue can be closed.

The Cache API is something completely different in my opinion. So it doesn't seem right to close this issue because the Cache API was implemented.

What the issue author asked was for fetch to automatically cache responses based on e.g. the Cache-Control header (basically to match the spec).

Let's say I keep fetching https://www.googleapis.com/oauth2/v3/certs to keep the public keys updated (whenever I use them). In the browser I know that this response will be cached until not "fresh" anymore (see this documentation), hence I know that I'm not going to be hammering that server with requests.

Using the Cache API I must myself parse the headers to understand how to properly cache the response (if I understand correctly), which would be a lot of work and not really related to this issue in my opinion.

Actually, yes, you're right. The closure of this issue appears to be a mistake.

@lino-levan
Copy link
Contributor

lino-levan commented Sep 17, 2023

Not sure how I missed that 😅. Definitely would be a nice-to-have feature. Fetch in Deno should work as closely to browsers as possible (outside of maybe allowing the Cookie header).

@JoakimCh
Copy link

It's still closed. Let's begin by opening it?

@tmikaeld
Copy link

tmikaeld commented Oct 15, 2023

@JoakimCh When an issue is closed, you often have to @lino-levan mention them directly.

I'd be very interested in respecting the cache headers for this feature, since it could enable fully automatic disk cache for things like signed S3 urls that has the necessary headers.

Would be even nicer if keys could get deleted after expiry, but afaik that's not part of the specs

@NfNitLoop
Copy link

I'm looking for this feature as well. I can imagine a few reasons why the Deno team might not want to make it the default (ex: why cache something downloaded by a script that runs only once.) I'd be fine with making the caching opt-in.

It could even be implemented in std first instead of core to see if there's enough adoption to merit RIIRing it. Something like const fetch = new CachingFetch({config}).

@JoakimCh
Copy link

ex: why cache something downloaded by a script that runs only once

This is actually a very interesting discussion!

But the most important thing for me is NOT persistency, but instead to have a runtime HTTP cache to avoid hammering a server when the data should be cached (as defined by the HTTP protocol).

But I think that maybe there should be a permission flag to allow a HTTP cache to persist outside of the script runtime e.g. --allow-http-cache-persistence with optional =size?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat new feature (which has been agreed to/accepted)
Projects
None yet
Development

No branches or pull requests