[V6] fix navigate is called synchronously in render #7203
Conversation
|
This will also fix #7199 |
|
The use of I'm not sure what the right fix for this is right now, but this PR is not it. |
| navigate(to, { replace, state }); | ||
| React.useEffect(() => { | ||
| navigate(to, { replace, state }); | ||
| }, [navigate, replace, state, to]); |
There was a problem hiding this comment.
It breaks synchronous redirects on the initial render.
There was a problem hiding this comment.
It shouldn't be synchronous, though. If there is a Redirect rendered, that navigation should happen in a second render pass. That also lets CM break up that work between frames (or not).
There was a problem hiding this comment.
What about useLayoutEffect? After all that's what it's able to do: synchronous side effects.
There was a problem hiding this comment.
useLayoutEffect happens after DOM manipulations, so effectively the render has already occurred. Also, those will produce errors in SSR, so you have to build detection to swap out for useEffect (which is a whole can of worms...).
There was a problem hiding this comment.
useLayoutEffect happens after DOM manipulations
I know that but it's still synchronous so that there won't be two different paints. I thought that was the original goal: to not paint a screen you'll redirect from.
There was a problem hiding this comment.
Sorry for the delay. After thinking about this a lot I think we just can't support redirect on the initial render once we get into the React lifecycle. In order to do this properly, we'd have to throw.
I'm going to change the tests around this and update our recommendation in v6 so people know if they need to redirect on the initial render, they should probably be doing it on the server.
Normally I'd agree, except React does sometimes allow you to setState in a function component. To demonstrate, I pushed a I was trying to take advantage of that same behavior inside that initial |
Yep, that's what it's doing. I just pushed another test to show how this works. The first test doesn't emit a warning. The second one does. |
- Removes <Redirect>
- Removes support for { redirectTo } in useRoutes()
- Warns if navigate() is used during the initial render
- Warns when <Navigate> is used on the initial render in <StaticRouter>
This began as a discussion on GitHub (see #7203). Essentially we need to
treat all navigation as a side effect, which means it needs to happen in
a useEffect. If people need v5's <Redirect> functionality (when it was
used in the route config inside a <Switch>), they either need to a) do
the redirect on the server (the best solution) or b) render a <Navigate>
on the initial render. However, (b) will only show the new route on the
subsequent render.
This should also eliminate warnings in the dev-experimental builds.
Closes #7203
Fixes #7199
|
This was fixed in cbcd398 |
The issue was found when router tries to do initial redirect navigation when rendering, but it in fact resulting in a synchronous
setStatesomewhere. Put the call in effect fix this issue.