Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent endless redirect #82

Open
Prinzhorn opened this issue Aug 18, 2021 · 7 comments
Open

Prevent endless redirect #82

Prinzhorn opened this issue Aug 18, 2021 · 7 comments

Comments

@Prinzhorn
Copy link

Prinzhorn commented Aug 18, 2021

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
Copy link
Owner

AlexxNB commented Aug 18, 2021

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

@Prinzhorn
Copy link
Author

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
Copy link
Owner

AlexxNB commented Aug 19, 2021

How to detect this case?

<Route path="/bar" redirect="/foo"/>
<Route path="/foo" redirect="/bar"/>

@Prinzhorn
Copy link
Author

Prinzhorn commented Aug 19, 2021

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
Copy link
Author

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

@AlexxNB
Copy link
Owner

AlexxNB commented Aug 19, 2021

  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
Copy link
Author

Prinzhorn commented Aug 20, 2021

  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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants