-
Notifications
You must be signed in to change notification settings - Fork 173
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
fix: correctly set Dispatcher
prototype for ProxyAgent
#451
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,6 +92,8 @@ export async function fetchUrlStream(input: string | URL, init?: RequestInit) { | |
return stream; | ||
} | ||
|
||
let ProxyAgent: typeof import('undici').ProxyAgent; | ||
|
||
async function getProxyAgent(input: string | URL) { | ||
const {getProxyForUrl} = await import(`proxy-from-env`); | ||
|
||
|
@@ -100,11 +102,20 @@ async function getProxyAgent(input: string | URL) { | |
|
||
if (!proxy) return undefined; | ||
|
||
// Doing a deep import here since undici isn't tree-shakeable | ||
const {default: ProxyAgent} = (await import( | ||
// @ts-expect-error No types for this specific file | ||
`undici/lib/proxy-agent.js` | ||
)) as { default: typeof import('undici').ProxyAgent }; | ||
if (ProxyAgent == null) { | ||
// Doing a deep import here since undici isn't tree-shakeable | ||
const [api, Dispatcher, _ProxyAgent] = await Promise.all([ | ||
// @ts-expect-error internal module is untyped | ||
import(`undici/lib/api/index.js`), | ||
// @ts-expect-error internal module is untyped | ||
import(`undici/lib/dispatcher/dispatcher.js`), | ||
// @ts-expect-error internal module is untyped | ||
import(`undici/lib/dispatcher/proxy-agent.js`), | ||
]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we doing all that just to reclaim some bytes? It seems a little dangerous 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 304000 of them but yes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added tests, hopefully those ensure we'll catch bugs if this hack breaks again in the future. |
||
|
||
Object.assign(Dispatcher.default.prototype, api.default); | ||
ProxyAgent = _ProxyAgent.default; | ||
} | ||
|
||
return new ProxyAgent(proxy); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these need to be loaded in a particular order?
I would recommend putting them in a separate file and just exposing the ProxyAgent from that file and add a comment on why those files are needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, any order, that's why I'm loading them concurrently.