Open steinybot opened 5 months ago
Even the naive code that recreates the router every time the environment changes still doesn't pickup the new environment which is even more concerning:
export function MyRouter() {
const environment = useRelayEnvironment();
console.debug('MyRouter', getId(environment));
const routes = preparePreloadableRoutes(MY_ROUTES, {
getEnvironment() {
console.debug('preparePreloadableRoutes getEnvironment', getId(environment));
return environment;
},
});
const router = createBrowserRouter(routes);
return <RouterProvider router={router} />;
}
Reproduction: https://github.com/steinybot/react-router-relay/blob/bug/stale-environment-2/examples/todo/src/MyRouter.tsx
I printed a unique id for each environment and you can clearly see here that when the entry point root rerenders it has a query with a stale environment:
This is a bit strange. getPreloadProps
is definitely being called and the query has the new environment but when the RouterProvider
re-renders it has the old state.
The first issue that I have noticed is that startNavigation
is called and it will call the loader
. RouterProvider
will render before completeNavigation
has been called and it will have an empty state
. Control will return back to startNavigation
and then completeNavigation
then updateState
is called which will call setState
in the RouterProvider
and it will now have the correct loaderData
. Now RouterProvider
will rerender and pass the loaderData
down to the route as expected. During first load, this intermediate render is fine, there is no state yet and so the route is not rendered. The issue is that when the environment changes and the router is recreated, this intermediate render will have the old state
and so the route will render with the previous query.
I haven't tracked down exactly why startNavigation
is yielding and allowing the RouterProvider
to render but that might be an issue with React Router itself.
What I haven't figured out yet is why setState
in RouterProvider
is not being called with the new loaderData
.
Hmm I'm not sure what has changed but now I am getting a 3rd render which does include the updated state. The intermediate renders with the old state are going to be problematic.
This will be fixed in https://github.com/remix-run/react-router/pull/11301. The example will still be wrong. You must create a new router when the environment changes. The router state must go back to initialized = false
to prevent the routes from being rendered with old data.
A workaround is to put a key on the RouterProvider
:
function usePrevious<A>(a: A): A | undefined {
const previousRef = useRef<A | undefined>(undefined)
useEffect(() => {
previousRef.current = a
})
return previousRef.current
}
function useChangeBit<A>(a: A): boolean {
const previous = usePrevious(a)
const previousChangeBitRef = useRef<boolean>(false)
const isSame = Object.is(previous, a)
const changeBit = isSame ? previousChangeBitRef.current : !previousChangeBitRef.current
useEffect(() => {
previousChangeBitRef.current = changeBit
});
return changeBit
}
function useChangeKey<A>(a: A): Key {
const changeBit = useChangeBit(a)
return changeBit ? 1 : 0
}
export function MyRouter() {
const environment = useRelayEnvironment();
// If the environment changes then flip the bit.
const environmentChangeKey = useChangeKey(environment)
console.debug('MyRouter', getId(environment));
const routes = preparePreloadableRoutes(MY_ROUTES, {
getEnvironment() {
console.debug('preparePreloadableRoutes getEnvironment', getId(environment));
return environment;
},
});
const router = createBrowserRouter(routes);
return <RouterProvider key={environmentChangeKey} router={router} />;
}
I just spent about half a day trying to figure out why the workaround wasn't working. preparePreloadableRoutes
mutates the passed in routes and sets the loader
which includes the environmentProvider
. When preparePreloadableRoutes
is called again it will not update the loader
to use the new environmentProvider
meaning that it retains the old environment
in its closure. That is so evil 🤬.
That'll be why the original example in the readme uses a ref and does not update it purely in a useEffect like it really ought to. render
must be pure and that means no updating refs.
So the example used a ref just to avoid recreating the routes and recreating the router on environment changes. It sounds like we do always need to do that though. In our production system it looks like we do recreate the router when the environment changes. Definitely open to changing the readme to better outline changing the environment.
preparePreloadableRoutes mutates the passed in routes and sets the loader which includes the environmentProvider. When preparePreloadableRoutes is called again it will not update the loader to use the new environmentProvider meaning that it retains the old environment in its closure.
I'm not sure I follow this part, preparePreloadableRoutes
shouldn't be mutating anything, it creates a new set of routes based on what was passed in (the code is pretty simple), so it shouldn't leak things between different calls or modifying the passed in routes.
Appologies for coming across as a jerk. I was frustrated but I shouldn't have blurted it out here.
I don't actually remember exactly what it was that was mutating. I think it is the children
. If the given route is not entry point route then newRoute
gets set to route
and then the children get overriden so that will mutate.
The example code in the README for how to change the environment does not work.
The queries are not reloaded and relay will print the following warning:
See a reproduction here: https://github.com/steinybot/react-router-relay/blob/bug/stale-environment/examples/todo/src/MyRouter.tsx