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

Adds mikeal/tunnel-agent to replace the global http.Agent when using a #128

Closed
wants to merge 1 commit into from

Conversation

zetlen
Copy link

@zetlen zetlen commented Apr 28, 2015

Adds mikeal/tunnel-agent to replace the global http.Agent when using a proxy. Resolving OSX Charles Proxy issue I mentioned in #86.

This does add another dependency, but tunnel-agent itself has no other deps, so we stay lean. I tried to keep your needle sharp.

@tomas
Copy link
Owner

tomas commented Apr 28, 2015

Thanks for the PR. I'll take a look into this later today (hopefully). On first sight it looks good but I'm not sure if this is the kind of stuff Needle should handle. Do you think it's possible to come up with a simpler solution that doesn't rely on the allowed_proxy_headers logic?

@zetlen
Copy link
Author

zetlen commented Apr 28, 2015

I think that a lower-level socket implementation is appropriate for this, and that's what an http.Agent is for.

However, I could maybe replace the regular "proxy" configuration with an "agent" configuration to get around this particular OSX issue. It would require hand-hacking an Agent, which means more and bulkier code on our end, but it would keep Needle more slim. Shall I try this and get back to you?

@tomas
Copy link
Owner

tomas commented Apr 28, 2015

Sure! As long as the solution Keeps it Simple(tm) while fixing the issue reported in #86, I think we're good. This PR is like half of the total LOCs on lib/needle.js!

@zetlen
Copy link
Author

zetlen commented May 1, 2015

Welp, I was able to get to a working state by just adding a tunnel auth specific to the local developer proxy we use. So if you think this PR is overkill, then please sweep it aside! Thanks for your attention in any case.

(fwiw I think the difference in loc is mostly coding style, not cyclomatic complexity or anything. I tried to emulate yours but I'm sure it could be tightened.)

@tomas
Copy link
Owner

tomas commented May 5, 2015

Hey @zetlen, no, thank YOU for the PR and reminding me of this issue. That make-local-proxy-agent.js file looks pretty nice and self-contained. How would you use that to get around the Charles proxy issue you were having with Needle?

@zetlen
Copy link
Author

zetlen commented May 5, 2015

We used to have a "recommended pattern" where if you want to use a local proxy, you could configure it like so, by setting some defaultRequestOptions that would be passed to Needle:

client.defaultRequestOptions = {
  proxy: 'http://127.0.0.1:8888',
  rejectUnauthorized: false
}

Our unit tests, and other code that used the SDK, had a general convention where you could make the SDK route through Fiddler/Charles for diagnostic purposes using an environment variable, as shown in this old version of the test setup.

Those options alone worked fine for Fiddler on Windows, but for the cross-platform Charles Proxy they caused the issue I described above.

So to fix it, we decided to let the SDK respond directly to the environment variable USE_FIDDLER. Now, all the testing and application code that looks for that variable is gone; instead, the global request function just looks for that env variable and substitutes an agent created by the make-local-proxy-agent.js file. This covers the 99% case and lets us debug on all platforms, and it doesn't interfere with the normal function of Needle's proxy configuration. I'm pretty satisfied with it so far! However, it's particular to our needs and doesn't solve the general case of Needle and Charles.

Thanks for being willing to at least entertain a big PR. The underlying bug might be in Charles or OSX, after all!

And please let me know if you see anything about our Needle usage that seems unwise.

@tomas
Copy link
Owner

tomas commented Nov 25, 2016

@zetlen We're finally going for the "clean" approach, which means letting the developer replace the default agent using tunnel-agent if he or she wants to. It seems like the logical path for Needle as the goal is to keep it lean and mean. Thanks anyway for your work!

@tomas tomas closed this Nov 25, 2016
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.

None yet

2 participants