-
Notifications
You must be signed in to change notification settings - Fork 15
Drop request and request-promise deps
#19
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
base: main
Are you sure you want to change the base?
Conversation
e67cd19 to
14f9665
Compare
2d5583f to
be88200
Compare
be88200 to
ec99dab
Compare
request dependency in favor of core https libraryrequest and request-promise deps in favor of request-stream
a0fbb61 to
e4ecc77
Compare
request and request-promise deps in favor of request-streamrequest and request-promise deps
e4ecc77 to
1693f22
Compare
avh4
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed elsewhere, I think we ought to continue to support redirects.
|
@avh4 Added redirect logic (it will follow up to 10 redirects; easy to adjust if desired) and some tests! |
|
I tried fixing the merge conflicts and got a failing test now: |
|
Do we need to “handle HTTPS_PROXY and NO_PROXY for people with corporate firewalls”? elm/compiler@41ec49e#diff-234dbb95568376fa69c8c8f02316f97a EDIT: Interesting module: https://github.com/gajus/global-agent |
See elm-lang/elm-platform#231 for motivation! This would drop about 4MB off every installer that uses it, which in turn would make them install faster. 🚀
This originally replaced
requestandrequest-promisewithrequest-stream, which is more lightweight than either. It's a tiny wrapper around Node's built-inhttpandhttpsmodules which adds support for passing in URLs (Node's core lib requires separately specifying hostname, port, protocol, etc) and for following redirects, and that's about it.Then I took it a step further and removed
request-streamin favor of directly using the built-inhttpandhttpsmodules. This has the downside of not following redirects, but it's not much work (or much code) to re-add support for that if it seems important. (I defaulted to assuming it's not, based on howbinwrapis used in practice.)Here are the dependencies post-PR:
I recommend viewing this PR with
?w=1.