Fix usage of Navigate in strict mode when using a data router #10435
Conversation
🦋 Changeset detectedLatest commit: 7f83a20 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| if (dataRouterState && dataRouterState.navigation.state !== "idle") { | ||
| return; | ||
| } |
There was a problem hiding this comment.
We no longer need this now that the effect has a proper dependency array - since it won't re-fire on the second render because the path own't have changed.
| React.useEffect( | ||
| () => navigate(JSON.parse(jsonPath), { replace, state, relative }), | ||
| [navigate, jsonPath, relative, replace, state] | ||
| ); |
There was a problem hiding this comment.
The reason this was problematic in 6.11 is that we would call navigate twice and when in a RouterProvider we'd delay resolving the relative path until inside the router. And thus the second execution would re-resolve to against the current location (which in a on-loader scenario would already be updated from the first effect).
Instead, we resolve the absolute path here in Navigate so that duplicate calls to navigate via the data router go to the same path - just as they do in BrowserRouter.
There was a problem hiding this comment.
I'm a little worried about state being in there. I'd still like to see this be an empty array as I think we are going to see bad app/library provider code that immediately flows new state / props down causing a new state identity due to no-one ever in the history of app code memoing the value they pass in. If tests are passing across react v17 and 18 though I'm fine shipping it.
There was a problem hiding this comment.
For future reference we tested the unit test setups from 93ccb2b in demo apps using react 16.8 and 18 and confirmed the UI was the same (even if the underlying React.StrictMode execution approaches differed). We may try to see if we can get our test suite running against both versions in a separate undertaking.
There was a problem hiding this comment.
Also, I think we're no worse off than we were previously where the effect had no second param since it would have re-run every time anyway?
There was a problem hiding this comment.
I'm a little worried about
statebeing in there. I'd still like to see this be an empty array as I think we are going to see bad app/library provider code that immediately flows new state / props down causing a new state identity due to no-one ever in the history of app code memoing the value they pass in. If tests are passing across react v17 and 18 though I'm fine shipping it.
This wound up biting me. I had an object that I was passing to state that wasn't memo'd. I think the check for dataRouterState !== "idle" saved me prior to 6.11.1, but with this change I had a flurry of redirects (each calling a loader and reloading a file hundreds of times).
|
🤖 Hello there, We just published version Thanks! |
|
🤖 Hello there, We just published version Thanks! |
Closes #10417