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

Jump to bottom

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

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

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

wants to merge 4 commits into from

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Jan 31, 2025 •
edited
Loading

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

@frandiox frandiox requested review from a team as code owners January 31, 2025 10:11
@github-actions GitHub Actions
Copy link
Contributor

Coverage report

St.❔
 Category Percentage Covered / Total
🟡 Statements
75.47% (-0.01% 🔻)
 8995/11919
🟡 Branches
70.64% (-0.02% 🔻)
 4387/6210
🟡 Functions 75.29% 2362/3137
🟡 Lines 75.98% 8497/11183
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%
🟢
... / ConcurrentOutput.tsx
98.36% (-1.64% 🔻)
88% (-4% 🔻)
 100%
98.33% (-1.67% 🔻)

Test suite run success

2032 tests passing in 909 suites.

Report generated by 🧪jest coverage report action from 8e3f962

karreiro
karreiro reviewed Jan 31, 2025
View reviewed changes
Copy link
Contributor

@karreiro karreiro left a comment

Choose a reason for hiding this comment

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

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

packages/theme/src/cli/utilities/theme-environment/html.ts Outdated Show resolved Hide resolved
@frandiox frandiox requested a review from karreiro February 3, 2025 14:03
@frandiox
Copy link
Contributor Author

frandiox commented Feb 3, 2025 •
edited
Loading

@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
Copy link
Contributor Author

frandiox commented Feb 3, 2025

/snapit

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 3, 2025

🫰✨ 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.

@frandiox frandiox mentioned this pull request Feb 3, 2025
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karreiro karreiro Awaiting requested review from karreiro

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: 40X errors when using shopify theme dev
2 participants
@frandiox @karreiro