Add support for request options #29
Conversation
4405244 to
a60a11b
Compare
There was a problem hiding this comment.
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- deno.json: Language not supported
Comments suppressed due to low confidence (1)
src/utils.ts:66
- [nitpick] The shallow merge of requestOptions may not correctly combine nested objects (e.g., headers). Consider using a deep merge strategy if preserving or merging nested properties is desired.
return { ...base, ...config.requestOptions, ...(parameters.requestOptions as http.RequestOptions), };
There was a problem hiding this comment.
Copilot reviewed 14 out of 15 changed files in this pull request and generated no comments.
Files not reviewed (1)
- deno.json: Language not supported
Comments suppressed due to low confidence (1)
src/utils.ts:91
- Since setEncoding('utf8') is called, the 'data' event returns strings rather than Buffers. Consider changing the type annotation to 'string' or removing it.
resp.on("data", (chunk: Buffer) => {
There was a problem hiding this comment.
Copilot reviewed 14 out of 15 changed files in this pull request and generated no comments.
Files not reviewed (1)
- deno.json: Language not supported
Comments suppressed due to low confidence (2)
src/utils.ts:66
- [nitpick] Consider using a deep merge for the headers property if both config.requestOptions and parameters.requestOptions include headers. This would allow combining header objects instead of one entirely overriding the other.
return { ...config.requestOptions, ...(parameters.requestOptions as http.RequestOptions), ...basicOptions };
tests/utils_test.ts:136
- [nitpick] Consider refining the type conversion instead of using an 'as unknown as' cast for requestOptions. Updating the parameter type definitions could improve type safety.
const params = { q: "coffee", requestOptions: customOptions } as unknown as qs.ParsedUrlQueryInput;
| }); | ||
| }; | ||
|
|
||
| const req = https.get(options, handleResponse).on("error", handleError); |
There was a problem hiding this comment.
deno task test fails, whereas deno task test:ci passes, I assume this is because we are using https module and it's trying to hit localhost with secure protocol (https)
| const req = https.get(options, handleResponse).on("error", handleError); | |
| const protocolModule = options.protocol === "https" ? https : http; | |
| const req = protocolModule.get(options, handleResponse).on("error", handleError); |
| assertEquals(options.hostname, "serpapi.com"); | ||
| assertEquals(options.port, 443); |
There was a problem hiding this comment.
Let's make this dynamic based on the environment.
tests/utils_test.ts
Outdated
| ? { | ||
| hostname: "localhost", | ||
| port: 3000, | ||
| } | ||
| : { | ||
| hostname: "serpapi.com", | ||
| port: 443, | ||
| }; |
There was a problem hiding this comment.
Let's include protocol here
| ? { | |
| hostname: "localhost", | |
| port: 3000, | |
| } | |
| : { | |
| hostname: "serpapi.com", | |
| port: 443, | |
| }; | |
| ? { | |
| hostname: "localhost", | |
| port: 3000, | |
| protocol: "http", | |
| } | |
| : { | |
| hostname: "serpapi.com", | |
| port: 443, | |
| protocol: "https", | |
| }; |
tests/serpapi_test.ts
Outdated
| ? { | ||
| hostname: "localhost", | ||
| port: 3000, | ||
| } | ||
| : { | ||
| hostname: "serpapi.com", | ||
| port: 443, | ||
| }; |
There was a problem hiding this comment.
Let's include protocol here
| ? { | |
| hostname: "localhost", | |
| port: 3000, | |
| } | |
| : { | |
| hostname: "serpapi.com", | |
| port: 443, | |
| }; | |
| ? { | |
| hostname: "localhost", | |
| port: 3000, | |
| protocol: "http", | |
| } | |
| : { | |
| hostname: "serpapi.com", | |
| port: 443, | |
| protocol: "https", | |
| }; |
|
Thanks @Ovi 👍 I removed the test against |
Resolves #27
This PR was initially for supporting the proxy feature natively. Unfortunately, HttpsProxyAgent isn't supported in some of the older NodeJS versions that this library promises to support.
As a fallback, this PR allows users to pass their own
http.RequestOptionsso they can set theagentto be the proxy agent they want.