solidjs / solid-router

A universal router for Solid inspired by Ember and React Router
MIT License
1.16k stars 147 forks source link

Navigation between dynamic routes does not work as expected #264

Closed StarLederer closed 1 year ago

StarLederer commented 1 year ago

Describe the bug

Note: This is related to #34 but the situation seems to have changed since then and if what I am about to describe is still the intended behavior I think it needs to be mentioned in the reamde and a workaround should be provided.

Note: This issue has been edited because my original reproduction included a bug that made this worse, unfortunately, the core of the problem seems to be a design issue so it remains.

URL change to a sibling dynamic route is not treated as a navigation event. It seems like dynamic <Route>s are treated as a single route so navigating from one to another does not cause a re-match.

<Routes>
  <Route path="user" component={User} >
    {/* /user/0, /user/1, etc. are treated as the same route so it is impossible to 'navigate' between them */}
    <Route path=":id" component={View} />
  </Route>
</Routes>

This is arguably the intended behavior because only one <Route> is defined in the template but I believe that dynamic routes should "expand" into multiple routes and be treated as separate because this way the Lifecycles are more intuitive, proper view transitions using solid-transition-group or View Transitions API are possible, and this is the behavior implemented by other routing libraries (e.g. vue-router).

Your Example Website or App

https://github.com/StarLederer/reproduction-solid-router-routes

Steps to Reproduce the Bug or Issue

  1. Click any of the links in the <nav>
  2. Click any other link in the <nav>
  3. See that the the exiting page updated while animated navigation to a different page was expected

Expected behavior

As a user, I expected that clicking the link would take me to the relevant page but instead the current page updated with the relevant information.

As a developer, I expected that dynamic routes were logically separate and navigation between them triggered a re-match but they are logically the same route so instead of navigating between them the current page state is updated.

Screenshots or Videos

No response

Platform

Additional context

No response

StarLederer commented 1 year ago

Update: A better workaround was provided by a contributor. I recommend you use that instead,

Workaround

For those trying to achieve the same effect, I've found an elegant workaround but I still think this should be default behavior or part of solid-router, or be mentioned in the documentation.

Crete component:

/** Re-renders when contents of `useParams()` update. */
const RematchDynamic: Component<{
  component: Component
  on?: (params: Params) => any
}> = (props) => {
  const params = useParams()
  const [page, setPage] = createSignal<JSXElement>(props.component({}))

  const paramSignal = () => props.on ? props.on(params) : Object.values(params)

  createEffect(on(paramSignal, () => {
    setPage(() => props.component({}))
  }))

  return page
}

Wrap your Route's component in the one we just created:

<Routes>
  <Route path="user" component={User} >
    <Route path=":id" element={<RematchDynamic component={View} />} />
  </Route>
</Routes>

This should work at any amount of nesting with any number of route parameters but be aware that by default this will re-render component when any route parameter changes so if you navigate from something like /user1/asks/user2 to /user1/asks/user3 the first 'user' route will re-match. If you want to only re-match when a specific parameter changes you can pass a signal to the on prop.

<Routes>
  <Route path="/:userA" element={<RematchDynamic on={params => params.userA} component={Us} />}>
    <Route path="/asks/:userB" element={<RematchDynamic component={Them} />} /> {/* No need to use `on here because if any parent prop changes it will cause a rematch up the tree and re-render this anyway ` */}
  </Route>
</Routes>
kyeotic commented 1 year ago

To underline the issue here, this causes solid-transition-group to animate the exit and entrance of all items, as if both pages shared a list that was being mutually modified.

This isn't limited to just transitions (though that's the hardest problem to solve), all components update as if they are receiving new props. That's just not how page changing should work! A page change should unmount the old page and mount the new page.

onMount effects should fire! onCleanup effects should fire!

The current behavior is surprising in the worst way.

Brendan-csel commented 1 year ago

In cases when you do want to blow away a chunk of the DOM and re-run your components, you could try using the keyed option with Show...

<Show when={params.something} keyed><MyComponent></Show>

I believe that re-renders MyComponent whenever params.something changes at all - not just from truthy to falsy.

Playground example

kyeotic commented 1 year ago

That's pretty cool. Would be nice to include it as a <KeyedRoute> component to handle automatic teardown for parameterized routes.

StarLederer commented 1 year ago

First of all, the solution with keyed <Show> is so much better than what I wrote, omg, thank you! I knew about keyed but I used it for very different purposes, I'll actually have to look into why this works and why it wouldn't work with default <Show>. Very interesting discovery.

Second, I have a correction to make, what I described and claimed to be the default behavior of vue-router is actually not correct, not completely at least. vue-router does indeed just replace stuff in the v-dom or whatever but vue itself tries to reuse existing dom elements and by default "optimizes" the transitions out in most cases. At the same time, their documentation does have a section about transitions and they describe how to avoid this (which, interestingly enough, also involves "keying").

Finally, I still believe in what I originally said with this having first class support. I do see how the solution is very simple and would understand the maintainers not wanting to include <KeyedRoute> in the library but I'd add a section in the README about transitions, just like vue-rotuter, and mention how to make Solid transition between pages instead of updating existing dom.