[Theme] Fix 401 and 405 for certain routes in local dev

[frandiox]

WHY are these changes introduced?

[HOLD] -- This currently fails when the proxy logic gets a 401 or similar because H3 has already sent headers/status and we can't overwrite them.

Fixes #5103

WHAT is this pull request doing?

Certain routes can't be rendered by SFR (e.g. app routes) and we must proxy them instead. We don't know before hand all of these routes, so this PR is adding a fallback: if a rendering fails with 4xx (the observed status errors for this situation), we try to proxy it. If the proxy doesn't work, we forward the original render error.

How to test your changes?

• Try going to /account/logout with --verbose. It should show that render failed and proxying worked.
• Then try /not-found-path. It should return the rendering error after trying the proxy (see logs).

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

• n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
• Existing analytics will cater for this addition
• PR includes analytics changes to measure impact

Checklist

• I've considered possible cross-platform impacts (Mac, Linux, Windows)
• I've considered possible documentation changes

[github-actions]

Coverage report

St.❔	Category	Percentage	Covered / Total
🟡	Statements	75.61% (+0.01% 🔼)	9053/11974
🟡	Branches	70.86% (+0.02% 🔼)	4416/6232
🟡	Functions	75.4%	2375/3150
🟡	Lines	76.12% (+0.02% 🔼)	8549/11231

Show files with reduced coverage 🔻

St.❔	File	Statements	Branches	Functions	Lines
🟢	... / app-event-watcher.ts	95.18% (-1.2% 🔻)	86.49% (-2.7% 🔻)	95.45%	100%

Test suite run success

2044 tests passing in 913 suites.

Report generated by 🧪jest coverage report action from 69d37db

[karreiro]

Thank you for this PR, @frandiox! I've left only one minor comment about the html module :)

[frandiox]

@karreiro To fix the problems I had earlier, I've refactored our proxy and render logic to use the native fetch and drop node-fetch and h3's sendProxy. I would suggest reviewing this commit by commit 🤞

Generally, I've tried to move away from h3 v1 utilities as much as possible. Turns out they support returning Responses in v1 as well, and I think it's their default in v2.

[frandiox]

/snapit

[github-actions]

🫰✨ Thanks @frandiox! Your snapshot has been published to npm.

Test the snapshot by intalling your package globally:

pnpm i -g @shopify/[email protected]

After installing, validate the version by running just shopify in your terminal
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

[karreiro]

Thank you, @frandiox! Excellent changes — we're going to have much more control over proxy requests with this change! 🚀

I've 🎩ed the proxy changes on shopify theme dev and shopify app dev 🔥

I've left two minor comments about:

1. Presenting the request IDs in some scenarios.
2. Preventing a warning from rendering in the scenario mentioned.

With those two minor changes applied, I believe this PR will be ready to be shipped!

[karreiro]

We're getting a 404 here when running the development server via shopify app dev. I've raised an issue for supporting the final fix for this.

While we don't have that fixed, can we prevent the warning from being rendered here when the app_block_id query parameter is passed? (Alternatively, instead of preventing it, we could still render the warning here in the verbose output in that context.)

[frandiox]

I believe you mean the warning in hot-reload/server instead of this one, right?

[karreiro]

Precisely, I believe that's very nice to have, but we may also apply that fix in another PR, as it transcends a bit the scope of this one.