Skip to content
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

Support got agent option #39

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

showi
Copy link

@showi showi commented Nov 23, 2022

Hello,
We want to be able to use a proxy so it will be nice to be able to pass the Agent option of GOT.

@hildjj
Copy link
Owner

hildjj commented Nov 23, 2022

I'm interested in taking this.

Can you please write a test for this? Please add yourself to the contributors section in package.json. This will land after #38, so there will probably need to be a rebase before we're done.

@@ -55,6 +55,8 @@ export class DNSoverHTTPS extends DNSutils {
* @param {Writable} [opts.verboseStream=process.stderr] Where to write
* verbose output.
* @param {boolean} [opts.http2=false] Use http/2 if it is available.
* GOT Agent objects
* @param {object} [opts.agent=Agent] Pass down got agent option

Choose a reason for hiding this comment

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

Suggested change
* @param {object} [opts.agent=Agent] Pass down got agent option
* @param {object} [opts.agent=Agents] Pass down got agent option

just to match the real type name

@showi
Copy link
Author

showi commented Nov 24, 2022

Hello,
I have test it locally by hacking the client to use a proxy (BURP).
I have a hard time figuring out how to test the proxy within nock.
We are basically just passing the option to got so... but i understand the requirement of the tests.
Maybe you have some guidance :)

@showi showi force-pushed the feature/support-got-agent-option branch 3 times, most recently from 8c216fc to 69171f7 Compare November 24, 2022 14:10
@showi showi force-pushed the feature/support-got-agent-option branch from 69171f7 to ab665d2 Compare November 24, 2022 14:11
@@ -55,6 +55,8 @@ export class DNSoverHTTPS extends DNSutils {
* @param {Writable} [opts.verboseStream=process.stderr] Where to write
* verbose output.
* @param {boolean} [opts.http2=false] Use http/2 if it is available.
* GOT Agent objects
* @param {object} [opts.agent=Agents] Pass down got agent option
Copy link
Owner

@hildjj hildjj Nov 25, 2022

Choose a reason for hiding this comment

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

Shouldn't this be @param {got.Agents|false} [opts.agent=false]?

@hildjj
Copy link
Owner

hildjj commented Nov 25, 2022

I don't think we need an end-to-end proxy test. It would be enough to do a simple pass-through Agent like:

class TestAgent extends http.Agent {
  constructor(options) {
    super(options)
    this.called = 0
  }
  createConnection(options, listener) {
    this.called++
    super.createConnection(options, listener)
  }
}

then assert that called has the correct value.

@showi
Copy link
Author

showi commented Nov 28, 2022

It seems that i can't just pass the agent, my guess is that nock has already override http.request so the agent instance is never called.

@Mikescops
Copy link

Hey, @hildjj can you help with the tests?

Indeed it seems that nock is intercepting the got call before, so using a custom TestAgent doesn't work.

Probably using mitm package would be better to intercept the network call and not the function call (but I might be mistaken as I'm not used to nock).

Thanks!

@hildjj
Copy link
Owner

hildjj commented Sep 3, 2024

I'm not going to take this for the release I'm working on right now. After I get the release out, we can rebase this and see if we can figure out a way to test it.

Dropping got in favor of the built-in fetch implementation is probably correct, but that moves us to requiring node 20+, which I'm not quite ready to do.

@hildjj
Copy link
Owner

hildjj commented Sep 9, 2024

ok, if you'd be willing to rebase this without adding anything else, I'm ready to work on getting it tested.

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.

3 participants