Fix theme dev ignoring SHOPIFY_HTTP(S)_PROXY - #8530
Open
ujjawal-yadav wants to merge 2 commits into
Open
Conversation
`createGlobalProxyAgent` patches Node's http/https agents, but the storefront request path called the built-in fetch (undici), which does not use them, so every render and proxied asset bypassed the proxy. Route both through `fetch` from `@shopify/cli-kit/node/http`, keeping the built-in `Response` at the H3 boundary, dropping the body on null-body statuses (node-fetch always reports a stream there) and keeping repeated `set-cookie` headers separate. Fixes Shopify#5890
Author
|
"I have signed the CLA!" |
The client in `@shopify/cli-kit/node/http` decompresses only exact `gzip`, `x-gzip`, `deflate`, `x-deflate` and `br` codings. A chained value such as `br, gzip` reaches us still compressed, and the unconditional `content-encoding` delete then relabelled it as identity, so the browser was handed compressed bytes it would not decode. The built-in fetch decoded chained codings, so this was a narrowing introduced by routing through the client. Also drop `101` and `103` from the null-body statuses. The `Response` constructor rejects any status outside 200-599, so neither can be built here whatever the body is.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
Fixes #5890.
shopify theme devignoresSHOPIFY_HTTP(S)_PROXY, so behind a corporate proxy every page render fails with the 502 /TypeError: fetch failedfrom the issue.bootstrap.tssets the proxy up withglobal-agent, which patches Node'shttp/httpsagents. The storefront path never reaches them: both branches ofrender()instorefront-renderer.tsandproxyStorefrontRequest()inproxy.tscall the built-infetch, which is undici and ignores those agents. Each carries aneslint-disable-next-line no-restricted-globals, and the rule's own message is the diagnosis: "Built-in fetch does not support HTTP proxies." Confirmed against a local proxy: the built-infetchgoes direct,node-fetchgoes through it.WHAT is this pull request doing?
Routes those three call sites through
fetchfrom@shopify/cli-kit/node/http, asstorefront-session.tsin the same directory already does, and drops the suppressions.Two details are why this isn't a one-line swap:
node-fetchResponse. Both functions are returned from H3 handlers, and returning the client's response as-is crashes the dev server (ERR_INVALID_ARG_TYPE ... Received an instance of Response). Both already re-wrapped withnew Response(response.body, response), so atoWebResponse()helper converts at that same spot.render()'s signature and its consumers are unchanged.node-fetchalways exposes a body stream, even on304. The built-in fetch reportsnullthere, andnew Response(stream, {status: 304})throws, so every revalidated asset would answer 500.toWebResponse()also reads headers throughraw(), since iteratingnode-fetchheaders joins repeatedset-cookievalues and would merge the storefront's session cookies.Requests use
slow-requestto keep the built-in fetch's semantics here: no automatic cancellation, so a slow theme still renders, and no retries, since this path carries cart and checkout writes. The request body goes throughReadable.fromWeb(); a 4 MB POST arrives intact. Proxied assets now also pass through cli-kit'srunWithTimer, which retains twoperformance.measureentries per request, as every other cli-kit request already does.getProxyStorefrontHeaders()no longer forwards the browser'saccept-encoding, so the storefront only picks a coding the client decodes.content-encodingis dropped only when the client actually decompressed the body, so anything it passes through, such as a chainedbr, gzip, still reaches the browser labelled correctly.Not touched:
notifier.ts, since--notifytargets a user-chosen and usually local URL, andparseThemeFileContent()incli-kit/node/themes/api.ts, which has the same bug fortheme pulldownloads but sits in another package. Happy to follow up there.This also sits alongside #8287 rather than against it: the change routes theme through cli-kit's client instead of around it, which holds whichever implementation cli-kit uses.
toWebResponse()gets simpler there, since h3 accepts an undiciResponsedirectly.How to manually test your changes?
mitmdump -p 8080.SHOPIFY_HTTP_PROXY=http://127.0.0.1:8080 SHOPIFY_HTTPS_PROXY=http://127.0.0.1:8080 shopify theme dev --store <store>mainit fails with 502. With this change the render and asset requests appear in the proxy log and the page loads. Reload once more to exercise the304path.Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add