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

rewrite in co generators #5

Closed
wants to merge 5 commits into from
Closed

rewrite in co generators #5

wants to merge 5 commits into from

Conversation

jonathanong
Copy link
Contributor

rewritten with the following changes:

  • use cogent instead of super agent. we can switch back to super agent if it gets fixed.
    • handles gzip
    • handles redirects
  • use graceful-fs over fs
  • remove auth stuff (re-add after everything else works)
  • remove proxy stuff (re-add after everything else works)

to do:

  • readd auth stuff - i need tests
  • readd proxy stuff - i need tests
  • reimplement old api - changed it a little, but there's no need for that. we can change it later.

right now it works, but there are a lot of tests to be had. it's already much cleaner.

@TooTallNate
Copy link
Contributor

I'm down in concept, but please don't remove already working features. I spent a good amount of time making proxy and auth support consistent in this module.

@jonathanong
Copy link
Contributor Author

@TooTallNate i don't plan to ever merge it without readding auth and proxy support, but i do need tests to make sure it's working correctly.

@TooTallNate
Copy link
Contributor

@jonathanong Sounds good, just checking :)

@jonathanong
Copy link
Contributor Author

@TooTallNate please add tests :) also, auth/proxy support in cogent or fixing super agent in general would be nice too.

@jonathanong
Copy link
Contributor Author

@TooTallNate looking at your proxy stuff, it seems that all that's needed is options.auth = proxyAgent(uri, secure). is it that simple? also, i want to replicate your tests, but i don't know what's going on here: https://github.com/TooTallNate/superagent-proxy/blob/master/test/test.js#L56

@TooTallNate
Copy link
Contributor

@jonathanong I think you mean options.agent = proxyAgent(uri, secure). The proxy variable in the superagent-proxy tests are just the URI to the proxy server. I made it configurable in the tests in case I wanted to use a different proxy server for the tests.

We can also take a look at proxy to spawn a HTTP/HTTPS proxy server up for the tests (a little bit of confirmation bias when using this module... but better then relying on external proxy services IMO).

@jonathanong
Copy link
Contributor Author

cool. much easier than i thought. shouldn't be difficult. i was confused because it seems like you tested using a local proxy, but you don't have any comments or references on how i could create a local proxy as well. an proxy included with the tests (maybe even optional like how you're doing it right now) using proxy would be helpful.

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

3 participants