-
Notifications
You must be signed in to change notification settings - Fork 154
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.
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
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
Coverage report
Show files with reduced coverage 🔻
Test suite run success2066 tests passing in 922 suites. Report generated by 🧪jest coverage report action from 0d1d9ce |
There was a problem hiding this 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! It looks good to me and works as expected as well.
I've shared a minor suggestion regarding H3 events, but it is optional (I would love to hear your thoughts on that tho).
Thanks again for this PR!
* 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]) |
There was a problem hiding this comment.
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
WHY are these changes introduced?
Requests using Section Rendering API and such don't need the proxy fallback since we are sure they are supposed to go to SFR. This is a quick fix to make logs easier for an issue that @jamesmengo is debugging.
WHAT is this pull request doing?
How to test your changes?
Post-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist