fix(frontend): keep query param when changing routes #674
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rperry2174
reviewed
Jan 3, 2022
| @@ -30,7 +29,7 @@ try { | |||
|
|
|||
| ReactDOM.render( | |||
| <Provider store={store}> | |||
| <BrowserRouter history={history} basename={basename()}> | |||
There was a problem hiding this comment.
@petethepig wasn't there a reason you felt you needed BrowserRouter instead of Router ?
size-limit report 📦
|
Codecov Report
@@ Coverage Diff @@
## main #674 +/- ##
=======================================
Coverage 75.70% 75.70%
=======================================
Files 45 45
Lines 1592 1592
Branches 294 294
=======================================
Hits 1205 1205
Misses 358 358
Partials 29 29 Continue to review full report at Codecov.
|
kolesnikovae
pushed a commit
that referenced
this pull request
Jan 7, 2022
* add tests for changing routes and maintaining query params * fix(frontend): rollback to Router to maintain queryParams sync with store
korniltsev
pushed a commit
that referenced
this pull request
Jul 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #671
This was introduced when we added baseUrl support, where we replaced the barebones
RouterforBrowserRouter, which broke thereact-query-synclibrary, since it doesn't expose thehistoryobject which is handled under the hood.(see more details at Treora/redux-query-sync#13 (comment))
I spoke with @petethepig and the only reason we did that was to allow passing a
basename, whichBrowserRoutersupport.So I just replaced
BrowserRouterforRouterto getredux-query-syncworking (which is the may issue this PR resolves), and then adds the basename to the history object.The only thing here is that
basenamewas removed fromhistoryfrom version 5 and onwards (remix-run/history#810), so that may become a problem. But it should be fine for now since we have a simple test that covers the functionality.Some other thoughts:
BrowserRouterdoes not take ahistoryparameter (https://v5.reactrouter.com/web/api/BrowserRouter) which would probably raise an eyebrown.baseURLstuff. Already spoken with @petethepig previously about doing it when we have more time, but honestly, this is the kind of thing we will forget if we don't do it soon.