🌎 Allow https proxy from env var #933
Conversation
| @@ -10,7 +10,6 @@ export { | |||
| export { exec, makeExecutable } from './exec.js'; | |||
| export { clirun, tic } from './utils.js'; | |||
| export { isUrl } from './isUrl.js'; | |||
| export { Session, getSession } from './session.js'; | |||
There was a problem hiding this comment.
This super minimal session implementation isn't used anywhere (besides a single test file - easily replaced). I just removed it so we did not need to implement fetch in this package.
| @@ -111,7 +110,7 @@ export async function buildHtml(session: ISession, opts: any) { | |||
| // Fetch all HTML pages and assets by the template | |||
| await Promise.all( | |||
| routes.map(async (page) => { | |||
| const resp = await fetch(page.url); | |||
| const resp = await session.fetch(page.url); | |||
There was a problem hiding this comment.
I'm not really sure about the implications of using an HTTPS proxy for localhost requests... These potentially happen here (for building static HTML) as well as in myst-execute, when searching for jupyter sessions.
Do we need logic in fetch for these cases?
There was a problem hiding this comment.
yes sounds like we do? i.e. skip proxy requests for any localhost / 127.0.0.1 requests based on the url, and proxy all others?
| async fetch(url: URL | RequestInfo, init?: RequestInit): Promise<Response> { | ||
| const urlOnly = new URL((url as Request).url ?? (url as URL | string)); | ||
| this.log.debug(`Fetching: ${urlOnly}`); | ||
| if (this.proxyAgent && !LOCALHOSTS.includes(urlOnly.hostname)) { |
There was a problem hiding this comment.
I kept this localhost check super simple... We could try to catch more edge cases with something like this: https://github.com/Kikobeats/localhost-url-regex/blob/master/src/index.js and/or we could add a process.env.LOCALHOST or something to allow users to specify if they've changed it....
I feel like all of that is probably overkill though.
|
I tested basic functionality running against a simple localhost proxy - all seems to work ok. Good enough to release and see how it goes with "real" proxies, I think. |
|
@fwkoch what did you use for your local proxy in the end (realizing here that a basic http server won't cut it, as it won't handle CONNECT)? I've had trouble getting it to connect to |
This PR moves
fetchto a function onsessionto centralize the logic, then addsHTTPS_PROXYconsumption: #919