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: remove node-fetch dependency, use custom fetch implementation #8486
Conversation
async function sendRequest(url: URL, options: http.RequestOptions & { maxRedirects: number }, postData?: Buffer): Promise<Response>{ | ||
return new Promise<Response>((fulfill, reject) => { | ||
const requestConstructor: ((url: URL, options: http.RequestOptions, callback?: (res: http.IncomingMessage) => void) => http.ClientRequest) | ||
= (url.protocol === 'https:' ? https : http).request; |
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.
TODO for data urls?
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.
I believe we don't need them. Data url can be easily loaded directly in a page. We can always add it later if there is demand.
return result; | ||
} | ||
|
||
const redirectStatus = [301, 302, 303, 307, 308]; |
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.
nit: please move that before sendRequest, otherwise code navigation is weird.
status === 303 && !['GET', 'HEAD'].includes(method)) { | ||
redirectOptions.method = 'GET'; | ||
postData = undefined; | ||
delete redirectOptions.headers?.[`content-encoding`]; |
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.
redirectionOptions.headers is coming from the options. Do you ensure there that the user is passing the headers in lower-case?
Compared to node-fetch current implementation doesn't support decompression of the results yet. Also cookie handling on redirects will be added later.
#5999