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

Resolve infinite redirects, broken redirects for same paths with different params #85

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

joshpensky
Copy link
Member

@joshpensky joshpensky commented Sep 13, 2021

Description

In our render resolution of Guard, we included a method to resolve redirecting to the same path.

Unfortunately, this didn't actually account for a redirect to the same path with different parameters. To fix this, we now perform a check for infinite redirection and only perform redirects in the render

Related issues

Fixes #55

What this does

  • Adds a check for infinite renders in validateRoute and throws up an error if so
  • Removes path checking in routeRedirect case of render—now only renders the Redirect component
  • Update default error from 'Not found.' to rrg/error (to match other errors)

How to test

Following the example in #55:

  1. Pull down the repo locally and set up the dev server (npm i, npm run bootstrap, npm start)

  2. In src/router/index.tsx, add the following guard function:

const invalidName: GuardFunction = (to, from, next) => {
  const { name } = to.match.params;
  if (name === 'test') {
    next.redirect('/charmander');
  } else {
    next();
  }
};
  <GuardedRoute
    path="/:name"
    exact
    component={Detail}
-   guards={[waitOneSecond, detailBeforeEnter]}
+   guards={[invalidName, waitOneSecond, detailBeforeEnter]}
  />
  1. Visit http://localhost:3001/test. Confirm the redirect resolves correctly to /charmander
  2. Update the invalidName guard to cause an infinite redirect to /test
  3. Confirm the app doesn't infinitely redirect + break, but rather throws up an error page

Additional Notes

Currently the routes are only "matched" by their pathname—something that will be changed by #39 (or similar functionality changes from that)

@joshpensky joshpensky added the bug Something isn't working label Sep 13, 2021
@joshpensky joshpensky self-assigned this Sep 13, 2021
@joshpensky joshpensky changed the base branch from main to develop September 14, 2021 14:29
Comment on lines +139 to +140
const redirectPath =
typeof routeRedirect === 'string' ? routeRedirect : routeRedirect.pathname;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const redirectPath =
typeof routeRedirect === 'string' ? routeRedirect : routeRedirect.pathname;
const redirectPath = routeRedirect?.pathname || routeRedirect;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's actually a TypeScript thing! routeRedirect can either be an object or string, so we need to type-check it first before accessing the pathname property

There's also this way to write it if it's preferable!:

let redirectPath: string | undefined;
if (typeof routeRedirect === 'string') {
  redirectPath = routeRedirect;
} else {
  redirectPath = routeRedirect.pathname;
}

@joshpensky joshpensky mentioned this pull request Sep 16, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redirect to the same route with different params
2 participants