-
-
Notifications
You must be signed in to change notification settings - Fork 177
Description
Tested with:
- wouter 3.9.0
- React 18.3.1 and 19.2.3
Suppose I have routing configured like this:
const Comp1 = () => {
useEffect(() => alert(`Comp1 was rendered`));
};
const Comp2 = () => {
useEffect(() => alert(`Comp2 was rendered`));
};
<Switch>
<Route path='/1'><Comp1 /></Route>
<Route path='/2'><Comp2 /></Route>
</Switch>And I navigate as follows:
- Navigate to
/1. - This renders
Comp1in the first route and executes the effect: I see the alert "Comp1 was rendered". - Navigate on to
/2. - This renders
Comp2in the second route and executes the effect: I see the alert "Comp2 was rendered". - I press the browser's native "back" button.
- The URL in the browser immediately changes to
/1.
Expected behavior:
- The second route for
/2does not match anymore, soComp2in it won't render, the effect is not executed. - The first route for
/1now matches again. The effect for thatComp1is executed, I see the alert "Comp1 was rendered".
Actual behavior:
- The second route renders its children again,
Comp2's effect gets executed, I see the alert "Comp2 was rendered". - Then the first route renders its children,
Comp1's effect gets executed, I see the alert "Comp1 was rendered".
Note
This render of Comp1 should not happen, since the URL doesn't match its route anymore!
Cause analysis:
Analyzing this I saw that Switch loops over all its children to see which route matches. When one matches, it renders only that route (which is fine), but also (via cloneElement) passes down an extra prop match with information about the details of the match:
wouter/packages/wouter/src/index.js
Lines 335 to 348 in 2061eac
| if ( | |
| isValidElement(element) && | |
| // we don't require an element to be of type Route, | |
| // but we do require it to contain a truthy `path` prop. | |
| // this allows to use different components that wrap Route | |
| // inside of a switch, for example <AnimatedRoute />. | |
| (match = matchRoute( | |
| router.parser, | |
| element.props.path, | |
| location || originalLocation, | |
| element.props.nest | |
| ))[0] | |
| ) | |
| return cloneElement(element, { match }); |
Presumably, this is done for performance reasons: Since Switch already determined that the Route in question does match, there is no reason to have Route determine that same fact a second time.
wouter/packages/wouter/src/index.js
Lines 244 to 247 in 2061eac
| const [matches, routeParams, base] = | |
| // `match` is a special prop to give up control to the parent, | |
| // it is used by the `Switch` to avoid double matching | |
| match ?? matchRoute(router.parser, path, location, nest); |
The content of that match variable (or rather its first array element, the matches boolean) decides whether the Route's child is rendered or not:
wouter/packages/wouter/src/index.js
Line 254 in 2061eac
| if (!matches) return null; |
But I don't think this works when using the browser's back/forward buttons - the timing seems to be somehow different compared to using a <Link>:
- When using a
Link: The change seems to be controlled by React and processed "synchronously" and "in order" - "top down" if you will. TheSwitchgets rendered first, has a chance to react to the new location from theLink, and won't render the now-no-more-matchingRouteat all. - When using the browser's back button: The change is probably propagated "asynchronously" to all components, including the "old"
Routewhich is subscribed to the location viauseLocationFromRouter(). It re-renders, and it would not match anymore under the new location, but since it still has itsmatchprop which it got passed down fromSwitch, it believes that it does still match and does not re-check if there is a new location. So it renders its children - which won't become visible anymore becauseSwitchwill unmount theRoutesoon after, but theRoute's child component will execute any effects it has defined.
I don't really know how to circumvent this without breaking the performance/caching optimization that the match prop offers, but for my project this causes enough problems with browser back behavior that I'm going to change my Route implementation to something like this:
const RouteWithoutMatch = (props: RouteProps) => {
// @ts-ignore Do not let `Switch` pass a `match` prop to `Route`
const { match, ...restProps } = props;
return <Route {...restProps} />;
};Now the matching Route inside a Switch will have to do the matching again, but it does prevent the weird errors I'm seeing for unmounted components executing their effects (and with the wrong combination of params at that, because those come from the stale match prop).
Codesandbox with repro
See here for an example where this happens: https://codesandbox.io/p/github/cataclyst/wouter-bug-repro/main
NOTE THAT YOU HAVE TO USE THE "DEPLOYED" VERSION OF THE SANDBOX, not the internal preview browser. So when the server is running, click "Open externally" to open the real (https://<something>.csb.app/) URL and see the issue. It does not happen with the back button in Codesandbox's internal browser preview widget.
This also contains a path with a Route variant that leaves out the match prop, and you can see that everything is fine there.
Maybe someone has an idea how to fix this while keeping the performance optimization? Or maybe that optimization has little enough impact that it could be removed?