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

Drop request and request-promise deps #19

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

rtfeldman
Copy link

@rtfeldman rtfeldman commented Aug 20, 2018

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 request and request-promise with request-stream, which is more lightweight than either. It's a tiny wrapper around Node's built-in http and https modules 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-stream in favor of directly using the built-in http and https modules. 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 how binwrap is used in practice.)

Here are the dependencies post-PR:

├── [email protected]
├─┬ [email protected]
│ ├─┬ [email protected]
│ │ └── [email protected] deduped
│ ├─┬ [email protected]
│ │ ├── [email protected]
│ │ ├── [email protected] deduped
│ │ ├── [email protected] deduped
│ │ └─┬ [email protected]
│ │   └─┬ [email protected]
│ │     ├── [email protected]
│ │     ├─┬ [email protected]
│ │     │ ├── [email protected] deduped
│ │     │ └── [email protected]
│ │     ├── [email protected] deduped
│ │     ├─┬ [email protected]
│ │     │ └─┬ [email protected]
│ │     │   ├── [email protected]
│ │     │   └── [email protected]
│ │     ├─┬ [email protected]
│ │     │ └── [email protected] deduped
│ │     └── [email protected]
│ └── [email protected]
└─┬ [email protected]
  ├─┬ [email protected]
  │ ├── [email protected]
  │ └─┬ [email protected]
  │   └── [email protected]
  └─┬ [email protected]
    └── [email protected]

I recommend viewing this PR with ?w=1.

@rtfeldman rtfeldman changed the title Drop request dependency in favor of core https library Drop request and request-promise deps in favor of request-stream Aug 20, 2018
@rtfeldman rtfeldman force-pushed the remove-request branch 14 times, most recently from a0fbb61 to e4ecc77 Compare August 20, 2018 14:37
@rtfeldman rtfeldman changed the title Drop request and request-promise deps in favor of request-stream Drop request and request-promise deps Aug 20, 2018
avh4
avh4 previously requested changes Sep 17, 2018
Copy link
Owner

@avh4 avh4 left a 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.

@rtfeldman
Copy link
Author

@avh4 Added redirect logic (it will follow up to 10 redirects; easy to adjust if desired) and some tests!

@avh4 avh4 dismissed their stale review May 9, 2019 03:52

"Added redirect logic"

@avh4
Copy link
Owner

avh4 commented May 9, 2019

I tried fixing the merge conflicts and got a failing test now:

  1) binwrap
       passes tests when specified URLs do exist:
     ChildProcessError: Command failed: (cd test_app && npm test)
TypeError: Cannot read property 'on' of undefined
    at /Users/avh4/workspace/binwrap/test.js:61:17
    at new Promise (<anonymous>)
    at /Users/avh4/workspace/binwrap/test.js:58:18
    at process._tickCallback (internal/process/next_tick.js:68:7)
    at Function.Module.runMain (internal/modules/cjs/loader.js:745:11)
    at startup (internal/bootstrap/node.js:279:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:696:3)
npm ERR! Test failed.  See above for more details.
 `(cd test_app && npm test)` (exited with error code 1)
      at callback (node_modules/child-process-promise/lib/index.js:33:27)
      at ChildProcess.exithandler (child_process.js:296:5)
      at maybeClose (internal/child_process.js:962:16)
      at Process.ChildProcess._handle.onexit (internal/child_process.js:251:5)

@lydell
Copy link
Contributor

lydell commented May 30, 2020

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
EDIT2: That module is 1.12MB…
EDIT3: What if we spawned curl instead? curl (and tar!) is available on Windows these days. We could fall back to wget on Linuxes without curl.

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