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

Issue #588: allow a function supplying the agent to support in follow… #632

Merged
merged 1 commit into from May 5, 2019

Conversation

@edgraaff
Copy link
Contributor

commented May 1, 2019

This PR addresses issue #588. When the user wants to provide an agent option, the user has to decide (prior making the call to fetch) to provide a http or https agent. If request is for http and makes it redirect to https (or vice versa), the provided agent is incompatible and breaks further processing.
Based on the proposal, agent can now be a function which takes parsedURL as an argument.

A typical use would be:

const httpAgent = new http.Agent({ keepAlive: true });
const httpsAgent = new https.Agent({ keepAlive: true });

const options = {
  agent: url => url.protocol === 'https:' ? httpsAgent : httpAgent
};
Edgar de Graaff
@bitinn

This comment has been minimized.

Copy link
Owner

commented May 2, 2019

This looks good to me, solves issues related to url-based agent creation (not just http/https change during redirect, but more advanced management of connection pools too.)

@jimmywarting what's your take on this?

@jimmywarting

This comment has been minimized.

Copy link
Collaborator

commented May 2, 2019

Lgtm

@bitinn bitinn merged commit bf8b4e8 into bitinn:master May 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jimmywarting

This comment has been minimized.

Copy link
Collaborator

commented May 5, 2019

From the small readme changed it's unclear what argument the function takes and what you should return.

Aside from that I would almost want to get rid of the option to provide a agent and only have a callback function

- agent: null   // http(s).Agent instance (or function providing one), allows custom proxy, certificate, dns lookup etc.
+ agent: null   // function(url) { ... } that can return an Agent that allows for custom proxy, certificate, dns lookup etc.

Think it's better to have one way of solving things instead of having to deal with every scenario

@bitinn

This comment has been minimized.

Copy link
Owner

commented May 5, 2019

Aside from that I would almost want to get rid of the option to provide a agent and only have a callback function

That's a breaking change and I think largely an unnecessary break (saves only 2 lines).

PR welcomed to fix doc though. I prefer adding another paragraph below the code block.

@jimmywarting

This comment has been minimized.

Copy link
Collaborator

commented May 5, 2019

That's a breaking change and I think largely an unnecessary

Yea, i know and that would be bad. Agree that it's unnecessary.

PR welcomed to fix doc though. I prefer adding another paragraph below the code block.

or just an example block using agent? didn't see any in the readme...


I'm just a bit frustrated ATM on another project i'm refactoring. it tries to deal with a lot of unnecessary overhead instead of just accepting one form of input. kinda: is stream? is it a buffer? canvas? string? promise? callback function? object? blob? FileList? filePath? url? then do this or that and it magically unwrap itself sometimes sync and sometimes async

Now when i mention it, it kind sounds like our Body.js constructor 😅
but it's not node-fetch i'm lashing out on. my other project is way worse

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.