szmarczak / cacheable-lookup

A cacheable dns.lookup(…) that respects TTL :tada:
MIT License
193 stars 29 forks source link

Make `.globalAgent` use `cacheable-lookup` #4

Closed Kikobeats closed 4 years ago

Kikobeats commented 5 years ago

Hello,

I love this library and I'm waiting for the next got version for starting using it, thanks for creating it 🙂

However, although most of my code is using got, still I feel I'm losing the opportunity for caching some requests that are using other libraries.

so I thought, how risky could be monkey patch http/https to be sure a cacheable lookup is being used?

const http = require('http');
const shimmer = require('shimmer');
const CacheableLookup = require('cacheable-lookup');
const cacheable = new CacheableLookup();

shimmer.wrap(http, 'get', function (originalGet) {

  // a good opportunity to setup good DNS!
  dns.setServers([
    '1.1.1.1',
    '1.0.0.1',
    '2606:4700:4700::1111',
    '2606:4700:4700::1001'
  ]);

  // override `http.get` for always use `cacheable.lookup` by default
  return function (url, opts, cb) {
    return originalGel(url, {lookup: cacheable.lookup ...opts}, cb)
  };
});

How viable do you think could be this? 👨‍🔬

szmarczak commented 5 years ago

If shimmer mocks the require function, then you're good. Just remember to import got after mocking http.

Anyway I don't like that approach - it's too much hassle. Why don't you use something simpler, for example Got hooks? I mean the beforeRequest hook. You can change the options too.

Kikobeats commented 5 years ago

My point is, how to leverage the functionality on this library into the http underlayer without using an explicit require or involve a dependency that integrates this library?

szmarczak commented 5 years ago

You can't.

sindresorhus commented 4 years ago

I agree it could be useful to be able to globally inject this, as long as it's clearly documented it should not be done in reusable packages.

Something like:

const CacheableLookup = require('cacheable-lookup');

CacheableLookup.initGlobally();

Where initGlobally() would accept the same options as an instance.

szmarczak commented 4 years ago

See https://github.com/nodejs/node/issues/18795

sindresorhus commented 4 years ago

I think we should open an issue on Node.js then about adding an API to officially allow intercepting http.get calls. There are lots of use-cases for that, including caching, debugging, etc.

szmarczak commented 4 years ago

We cannot patch the http module, but we can override the global Agent.