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

install.js should support HTTPS_PROXY environment variable #347

Closed
silverwind opened this issue Aug 24, 2020 · 4 comments · Fixed by #1621
Closed

install.js should support HTTPS_PROXY environment variable #347

silverwind opened this issue Aug 24, 2020 · 4 comments · Fixed by #1621

Comments

@silverwind
Copy link

silverwind commented Aug 24, 2020

The esbuild module has below rudimentary fetch implementation that does not support fetching via HTTPS proxies which means the script will run for over 4 minutes waiting for the HTTPS request to timeout in environments without direct Internet connectivity.

I'd suggest switching to make-fetch-happen for this download which is a fetch-compatible module that npm itself uses and it has full proxy support.

function fetch(url2) {
  return new Promise((resolve, reject) => {
    https.get(url2, (res) => {
      if ((res.statusCode === 301 || res.statusCode === 302) && res.headers.location)
        return fetch(res.headers.location).then(resolve, reject);
      if (res.statusCode !== 200)
        return reject(new Error(`Server responded with ${res.statusCode}`));
      let chunks = [];
      res.on("data", (chunk) => chunks.push(chunk));
      res.on("end", () => resolve(Buffer.concat(chunks)));
    }).on("error", reject);
  });
}

Related: #319

I could probably send you a pull request but I could not find the source for this script in the repo.

@silverwind silverwind changed the title install.js should support HTTPS_PROXY environment variables install.js should support HTTPS_PROXY environment variable Aug 25, 2020
@evanw
Copy link
Owner

evanw commented Aug 26, 2020

I don't want the install script to depend on additional packages. I'm going to try moving npm install before the direct download. That should handle this case while still supporting the case where npm isn't installed, since the npm command should fail instantly if it doesn't exist.

@evanw evanw closed this as completed in 11ea03b Aug 26, 2020
@silverwind
Copy link
Author

silverwind commented Aug 26, 2020

What's the downside of doing npm install? Does it require golang on the system?

I guess if you don't want additional dependencies, you could replicate what these two modules are doing to support downloads via proxies:

https://github.com/Rob--W/proxy-from-env
https://github.com/gajus/global-agent

(It's a shame proxy support isn't in Node.js)

@evanw
Copy link
Owner

evanw commented Aug 26, 2020

The downside is that npm install is slower and more complicated than just doing a HTTP request because npm itself is slow and complicated. Running npm recursively is not exactly a supported use case (e.g. it deadlocks when doing a global install without a specific workaround). People may also have overridden the npm command with something else so it's impossible to know if it'll actually do what is intended or not. That's why I'm going to keep the HTTP request fallback path. I totally agree with you about node not having proxy support :(

@silverwind
Copy link
Author

Can confirm it's much faster now with the latest version. My CI now does only take 30s to install modules compared to 5 minutes before. Thanks.

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 a pull request may close this issue.

2 participants