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

Enhance the cache option #875

Open
2 of 6 tasks
szmarczak opened this issue Sep 16, 2019 · 28 comments
Open
2 of 6 tasks

Enhance the cache option #875

szmarczak opened this issue Sep 16, 2019 · 28 comments
Labels
💵 Funded on Issuehunt The issue has been funded on Issuehunt enhancement This change will extend Got features external The issue related to an external project ✭ help wanted ✭

Comments

@szmarczak
Copy link
Collaborator

szmarczak commented Sep 16, 2019

Issuehunt badges

The cache mechanism:

  • should be a promise
  • should be replaceable with any other module*
  • should have something hook-like to allow storing different properties e.g. ip addresses
  • should accept different caching logic
  • should make a new request when the cached response cannot be updated (Do not use invalid cache #963)
  • beforeCache hook

* Through the request option. Accepting ClientRequest and IncomingMessage as the return type. Return undefined to fallback. You can override the request option also by returning a ClientRequest or an IncomingMessage instance in a beforeRequest hook.

@sithmel
Copy link

sithmel commented Feb 15, 2020

About #746:
What about the approach of adding an hook that can override the headers (including the caching ones), before they are evaluated by cacheable-request ?
This way you can set different caching headers, tweaking the cache key (etag or even the url) etc.

@szmarczak
Copy link
Collaborator Author

@sithmel beforeCache hook? Sounds good to me.

@sithmel
Copy link

sithmel commented Feb 16, 2020

Happy to implement it as soon as this is done and merged

@sithmel
Copy link

sithmel commented Feb 18, 2020

@szmarczak I guess I should wait for #1051 to be merged ... do you agree ?

@szmarczak
Copy link
Collaborator Author

@sithmel You don't have to. Feel free to send a PR anytime, I'll merge with master later. No worries! :)

@MrSwitch
Copy link

Can i add another suggest feature which is kind of related to being a promise #1078

@rifler
Copy link
Contributor

rifler commented Jun 18, 2020

i've just read ky docs https://www.npmjs.com/package/ky#hooksbeforerequest

The hook can return a Request to replace the outgoing request, or return a Response to completely avoid making an HTTP request. This can be used to mock a request, check an internal cache, etc

sounds quite similar for #746 needs

@szmarczak
Copy link
Collaborator Author

@rifler It's already done. It hasn't been documented yet.

@szmarczak
Copy link
Collaborator Author

@rifler 779062a

@adityapatadia
Copy link

It would be great if you can also write how can we give custom request. It's still not clear from Readme.

@szmarczak
Copy link
Collaborator Author

@adityapatadia http.request returns a ClientRequest instance for example. I will write a more detailed tutorial soon.

@adityapatadia
Copy link

@adityapatadia http.request returns a ClientRequest instance for example. I will write a more detailed tutorial soon.

I understand the return type but I am not sure how can I return custom request function.

Do you mean something like this?

beforeRequest: [
			async options => {
				return new CacheableRequest(http.request);
			}
		]

@szmarczak
Copy link
Collaborator Author

No. I mean ClientRequest, not CacheableRequest:

const got = require('got');
const https = require('https');

got('https://example.com', {
    hooks: {
        beforeRequest: [
            () => {
				// `https.request` returns a ClientRequest instance
                return https.request('https://httpbin.org/anything');
            }
        ]
    }
}).json();

@szmarczak
Copy link
Collaborator Author

You can pass an IncomingMessage instance as well. To generate one you can use for example https://github.com/lukechilds/responselike

@adityapatadia
Copy link

Understood. Thanks for help.

@szmarczak
Copy link
Collaborator Author

No problem, if you got any other questions, feel free to ask :)

@hk7math
Copy link

hk7math commented Oct 30, 2020

May I know if #746 is possible yet? When remote host returns headers with {"cache-control": "no-cache"}, is there a way to override it without adding beforeRequest and afterResponse hooks to implement custom caching? Thank you very much!

@sindresorhus sindresorhus unpinned this issue Dec 10, 2020
@adityapatadia
Copy link

No problem, if you got any other questions, feel free to ask :)

Sorry to get back after long time but I tried this and it throws many errors related to agent and timeout.


beforeRequest: [async options => {
        return new Promise((resolve, reject) => {
          let cr;
          if(options.url.protocol == "https:"){
            cr = new cacheableRequest(https.request);
          } else {
            cr = new cacheableRequest(http.request);
          }
          cr(options).on("request", resolve);
        });
      }],

Basically there is no easy way to just edit some part of cacheableRequest module and then plug it in this library.

@szmarczak
Copy link
Collaborator Author

@adityapatadia You're creating a new CacheableRequest instance on every got call. You can do this instead:

const runCachedHttps = new CacheableRequest(https.request);
const runCachedHttp = new CacheableRequest(http.request);

...

beforeRequest: [async options => {
        return new Promise((resolve, reject) => {
          const cr = options.url.protocol == "https:" ? runCachedHttps : runCachedHttp;
          const instance = cr(options);
		  instance.on("request", resolve);
		  instance.on("error", reject);
        });
      }],

...

@StarpTech
Copy link

Anything we can do to push this feature further?

@KyleAMathews
Copy link

I had an issue where I couldn't trust the cache-control headers so edited them before saving the cached response gatsbyjs/gatsby#32012

@szmarczak
Copy link
Collaborator Author

I have started working on a new cache implementation. I will let you know when I have a working example.

@szmarczak
Copy link
Collaborator Author

Update: more than 50% RFC-compliant things finished.

@szmarczak
Copy link
Collaborator Author

Update: I just need to finish revalidation, conditional requests, tests and we're all set ✨

@chris-schra
Copy link

any news on this? :)

@szmarczak
Copy link
Collaborator Author

Sorry, been busy w/ work and uni. Will work on this asap.

@szmarczak szmarczak pinned this issue Dec 10, 2021
@vjpr
Copy link

vjpr commented Apr 26, 2022

Any progress on this?

I just want to be able to always cache a response...is there any hook I can use to do this?

@sindresorhus sindresorhus unpinned this issue Jul 5, 2022
@wilsonssss
Copy link

Any progress on this?

I just want to be able to always cache a response...is there any hook I can use to do this?

The same requirement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💵 Funded on Issuehunt The issue has been funded on Issuehunt enhancement This change will extend Got features external The issue related to an external project ✭ help wanted ✭
Projects
None yet
Development

No branches or pull requests