-
-
Notifications
You must be signed in to change notification settings - Fork 11.6k
Description
Describe the bug
Since upgrading to axios@1.4.0, our usage of cacheable-lookup stopped working. I have identified that this change is the cause.
The reason is because the lookup option is now explicitly included in the options object, set to the undefined value. This is propagated through all the way to the HTTP agent. The cacheable-lookup library naively checks for the presence of the property, rather than if it is set to a usable or expected value, thus causing the problem.
While I believe the fix should be made in cacheable-lookup, however we are unable to upgrade beyond v6 because sindresorhus decided to force everyone to use ESM, and we cannot migrate our whole codebase to support ESM just like that 😓 I imagine others are in the same boat.
I would really appreciate if axios could change the explicit inclusion of lookup to an "if present" style (e.g. ...(lookup ? { lookup } : {}),) to resolve this problem. Of course, I understand if you would prefer not to.
To Reproduce
- Install
axios@1.4.0and witness cached DNS is not used. - Downgrade to
1.3.6and witness it is used.
See code snipped for a test.
Code snippet
import https from 'https';
import { test, jest, expect } from '@jest/globals';
import axios from 'axios';
import CacheableLookup from 'cacheable-lookup';
test('cached dns', async () => {
const cachedDns = new CacheableLookup();
cachedDns.install(https.globalAgent);
jest.spyOn(cachedDns, 'lookupAsync');
const res = await axios.get('https://example.com');
expect(res.status).toBe(200);
expect(cachedDns.lookupAsync).toHaveBeenCalled();
});Expected behavior
Should always use cached DNS when installed.
Axios Version
1.4.0
Adapter Version
HTTP
Browser
N/A
Browser Version
N/A
Node.js Version
18.12
OS
All?
Additional Library Versions
No response
Additional context/Screenshots
No response