AlexxNB / tinro

Highly declarative, tiny, dependency free router for Svelte's web applications.
MIT License
669 stars 30 forks source link

Prevent endless redirect #82

Open Prinzhorn opened 2 years ago

Prinzhorn commented 2 years ago

I was just typing <Route path="/" redirect="/"> in REPL (for #81) and it froze my tab. I assume because of an endless redirect (my cursor was in the redirect prop and I was about to type there).

To reproduce open REPL and paste <Route path="/" redirect="/" />

It should detect that edge case and not run into an endless loop. Mistakes happen and if path or redirect are dynamic this would lock up an app.

Maybe add a console.error for this case in dev mode

AlexxNB commented 2 years ago

Any suggestion to solve this? I don't want to rise a timer for each redirect action.

Prinzhorn commented 2 years ago

Can you check if the to-be-redirected URL exactly matches the current URL (including hash, query and everything)? I doubt anyone wants to have the exact same URL twice in the history stack.

AlexxNB commented 2 years ago

How to detect this case?

<Route path="/bar" redirect="/foo"/>
<Route path="/foo" redirect="/bar"/>
Prinzhorn commented 2 years ago

I don't know if we need to go that far for a feature that's mainly meant to prevent users from shooting in their own foot.

An algorithm to solve this would be to build a graph of all redirects (/foo -> /bar and /bar -> /foo as two nodes with two edges) and check if it is free of cycles.

graph

Prinzhorn commented 2 years ago

I think as a first step checking path === redirect prop and then logging an error would be great.

AlexxNB commented 2 years ago
  1. Graph is impossible, because routes are unknown. Tinro knows only opened childs hierarchy.
  2. Size of graph algorithm may increase tinro size more than twice.
  3. Half solution is not a solution.
Prinzhorn commented 2 years ago
  1. I know nothing about tinro's architecture. Couldn't you hold the graph in <script context="module"> of <Route> and each Route adds/updates/removes itself?
  2. Like I said I don't think it's feasible either because it adds too much complexity (worst case it adds more bugs with false positives)
  3. Catching path === redirect would be one less bug