-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix(qwik-city): fix redirect with search params #6410
Conversation
👷 Deploy request for qwik-insights pending review.Visit the deploys page to approve it
|
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'm not sure this isn't a breaking change, since now it will re-run route loaders where it wasn't doing that before.
If we apply this, then there's no way to opt out of this behavior. If you are updating the search string often, it will cause a lot of network requests, no?
Perhaps there is a way to opt out of route loading when the search string changes?
@mhevery thoughts?
In my case this is expected behavior, because loader depends on the I can also confirm that there're might be use-cases when developers want to update URLSearchParams without triggering navigation, given that such discussions exist: remix-run/react-router#9851 so maybe having an option to opt-out is something to consider at least. Also, when I tested my solution, it seems like (even in dev mode when I tested it manually) Qwik will not re-send a request, and rather re-use what fetch already returned after the redirect. Am I right? |
Run & review this pull request in StackBlitz Codeflow. commit: @builder.io/qwik
@builder.io/qwik-city
eslint-plugin-qwik
create-qwik
|
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 tested your code locally and it's working fine.
This is a great fix btw. With the old code routeLoader$
is running without changing the URL and this is not correct at all.
With your fix @octet-stream the problem is solved.
Thanks 👍
Overview
I am not sure if my issue is related to #6103, so I decided open a PR with the fix for my issue. This PR changes how qwik-city-component handles URLSearchParams changes within same route segment.
What is it?
Description
This PR replaced
isSamePathname
withisSamePath
so that the URL will be updated after redirect.Use cases and why
Current behavior: When action redirects within same route segment, and only changes URLSearchParams, Qwik ignores the change of the URL, but the data on the page is updated.
After this change: The Qwik updates the URL, along with data refresh, just the way it works in browsers.
Reproduction demo can be found here: https://stackblitz.com/edit/qwik-redirects-er3vm8-36vtnr?file=src%2Froutes%2Flayout.tsx&view=editor
Checklist: