Skip to content

Add support for request options #29

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Add support for request options #29

wants to merge 12 commits into from

Conversation

zyc9012
Copy link
Collaborator

@zyc9012 zyc9012 commented Apr 3, 2025

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.RequestOptions so they can set the agent to be the proxy agent they want.

@zyc9012 zyc9012 force-pushed the support-proxy branch 2 times, most recently from 4405244 to a60a11b Compare April 5, 2025 10:11
@zyc9012 zyc9012 changed the title Add support for proxy Add support for request options Apr 5, 2025
@zyc9012 zyc9012 requested review from aciddjus, Ovi and Copilot April 7, 2025 01:54
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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), };

@zyc9012 zyc9012 requested a review from Copilot April 7, 2025 02:09
@zyc9012 zyc9012 self-assigned this Apr 7, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) => {

@zyc9012 zyc9012 requested a review from Copilot April 7, 2025 02:13
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link

@Ovi Ovi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @zyc9012! 👍
Great work.

Comment on lines -32 to +33
node-version: '18.x' # Build files using a fixed node version
registry-url: 'https://registry.npmjs.org'
node-version: "22.x" # Build files using a fixed node version
registry-url: "https://registry.npmjs.org"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bumping the Node.js version to 22.x for the build environment, should we also add Node.js v20.x and v21.x to the test matrices? This would help ensure compatibility with all current stable versions.

Comment on lines +125 to +127
```bash
npm install https-proxy-agent
```
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also show guide for yarn?

Suggested change
```bash
npm install https-proxy-agent
```
```bash
# If you use npm:
npm install https-proxy-agent
# Or if you use Yarn:
yarn add https-proxy-agent

Let's also update where we guide users to install serpapi:

npm install serpapi

# If you use npm:
npm install serpapi

# Or if you use Yarn:
yarn add serpapi

});
};

const req = https.get(options, handleResponse).on("error", handleError);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Suggested change
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);

Comment on lines +208 to +209
assertEquals(options.hostname, "serpapi.com");
assertEquals(options.port, 443);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this dynamic based on the environment.

Comment on lines +27 to +34
? {
hostname: "localhost",
port: 3000,
}
: {
hostname: "serpapi.com",
port: 443,
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's include protocol here

Suggested change
? {
hostname: "localhost",
port: 3000,
}
: {
hostname: "serpapi.com",
port: 443,
};
? {
hostname: "localhost",
port: 3000,
protocol: "http",
}
: {
hostname: "serpapi.com",
port: 443,
protocol: "https",
};

Comment on lines +44 to +51
? {
hostname: "localhost",
port: 3000,
}
: {
hostname: "serpapi.com",
port: 443,
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's include protocol here

Suggested change
? {
hostname: "localhost",
port: 3000,
}
: {
hostname: "serpapi.com",
port: 443,
};
? {
hostname: "localhost",
port: 3000,
protocol: "http",
}
: {
hostname: "serpapi.com",
port: 443,
protocol: "https",
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Support HTTPS_PROXY / HTTP_PROXY / NO_PROXY environment variables for proxying
2 participants