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.

Sign up for GitHub

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

[Themes] Skip proxy fallback for known rendering requests #5445

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

[Themes] Skip proxy fallback for known rendering requests #5445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25a7b58
Skip proxy fallback for known rendering requests
frandiox Feb 21, 2025
0d1d9ce
Changesets
frandiox Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/great-clocks-dress.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/theme': patch
---

Fix masking 404 errors as 200 when using Section Rendering API.
10 changes: 9 additions & 1 deletion packages/theme/src/cli/utilities/theme-environment/html.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export function getHtmlHandler(theme: Theme, ctx: DevServerContext) {
replaceTemplates: getInMemoryTemplates(ctx, browserPathname, getCookie(event, 'localization')?.toLowerCase()),
})
.then(async (response) => {
if (response.status >= 400 && response.status < 500) {
if (response.status >= 400 && response.status < 500 && !isKnownRenderingRequest(event)) {
// We have tried to render a route that can't be handled by SFR.
// Ideally, this should be caught by `canProxyRequest` in `proxy.ts`,
// but we can't be certain for all cases (e.g. an arbitrary app's route).
Expand Down Expand Up @@ -105,6 +105,14 @@ function createErrorPageResponse(
})
}

/**
* Detects routes and params that indicate this request should be handled by SFR.
*/
function isKnownRenderingRequest(event: H3Event) {
const searchParams = new URLSearchParams(event.path.split('?')[1])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this logic in other areas. I wonder if it might be nice to decorate™️ events to have this in a single place, with something like this:

event = devServer(event)
event.queryParams
event.pathname

return ['section_id', 'sections', 'app_block_id'].some((key) => searchParams.has(key))
}

async function tryProxyRequest(event: H3Event, ctx: DevServerContext, response: Response) {
outputDebug(
`Render failed for ${event.path} with ${response.status} (x-request-id: ${response.headers.get(
Expand Down
17 changes: 17 additions & 0 deletions packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,23 @@ describe('setupDevServer', () => {
await expect(eventPromise).resolves.toHaveProperty('status', 401)
})

test('skips proxy for known rendering requests like Section Rendering API', async () => {
const fetchStub = vi.fn()
vi.stubGlobal('fetch', fetchStub)
fetchStub.mockResolvedValueOnce(new Response(null, {status: 200}))
vi.mocked(render).mockResolvedValue(new Response(null, {status: 404}))

await expect(dispatchEvent('/non-renderable-path?sections=xyz')).resolves.toHaveProperty('status', 404)
await expect(dispatchEvent('/non-renderable-path?section_id=xyz')).resolves.toHaveProperty('status', 404)
await expect(dispatchEvent('/non-renderable-path?app_block_id=xyz')).resolves.toHaveProperty('status', 404)

expect(vi.mocked(render)).toHaveBeenCalledTimes(3)
expect(fetchStub).not.toHaveBeenCalled()

await expect(dispatchEvent('/non-renderable-path?unknown=xyz')).resolves.toHaveProperty('status', 200)
expect(fetchStub).toHaveBeenCalledOnce()
})

test('renders error page on network errors with hot reload script injected', async () => {
const fetchStub = vi.fn()
vi.stubGlobal('fetch', fetchStub)
Expand Down
Loading