Skip to content

allows for a cleaner way of handling authenticated routes#332

Closed
iamthesiz wants to merge 1 commit into
reach:masterfrom
iamthesiz:master
Closed

allows for a cleaner way of handling authenticated routes#332
iamthesiz wants to merge 1 commit into
reach:masterfrom
iamthesiz:master

Conversation

@iamthesiz

@iamthesiz iamthesiz commented Dec 4, 2019

Copy link
Copy Markdown

Allows for syntax like this

const Routes = () => {
  const { user } = useAuth()
  const handlePrivateRoute = navigate => {
    if (!user) navigate('/login')
  }
  return (
    <Router onPrivateRoute={handlePrivateRoute}>
      <SettingsPage path="/settings" private />
      <LoginPage path="/login" default />
    </Router>
  );
}

@iamthesiz

iamthesiz commented Dec 5, 2019

Copy link
Copy Markdown
Author

Just been thinking about this. Could also have a more generic use for this.

const Routes = () => {
  const { user } = useAuth()
  const handleRouteChange = ({ navigate, private: isPrivateRoute, ...props }) => {
    if (!user && isPrivateRoute) navigate('/login')
  }
  return (
    <Router onRouteChange={handleRouteChange}>
      <SettingsPage path="/settings" private />
      <LoginPage path="/login" />
    </Router>
  );
}

@frastlin

frastlin commented Dec 19, 2019

Copy link
Copy Markdown

I think this should be called a hook, and I think there should be an object passed to the hook that contains the URL the user is navigating to, and maybe specify the full URL as well as the relative URL.

@vertic4l

Copy link
Copy Markdown

@alex-cory great, that's exactly how i'd like to have it! The right spot to handle such stuff.

@blainekasten

Copy link
Copy Markdown
Collaborator

I don't foresee this router taking an active stance on how to handle authenticated vs non-authenticated routing. This is best solved in user-land as different cases require different setups. Thanks for the contribution though!

@iamthesiz

Copy link
Copy Markdown
Author

@blainekasten that's why I suggest the onRouteChange which is not specific to authenticated routes but pretty broad. Could have multiple use cases. :)

@gregbarcza

Copy link
Copy Markdown

+1 @blainekasten what is the problem with the onRouteChange approach?

@blainekasten

Copy link
Copy Markdown
Collaborator

Supporting an onRouteChange I believe is more complicated than it looks and could lead to not great user experiences (flicker of navigations and renders).

Like mentioned, this could be handled in user land with a composable hook that redirects.

@iamthesiz

iamthesiz commented Apr 2, 2020

Copy link
Copy Markdown
Author

Hm... I'm curious how it would cause a flicker. Is it because navigate is used outside the Router? I think in "user land" if someone decides to update state in their App component (where we might place the Router) and it causes a rerender, that's on them. Documentation can be provided about that. If they're changing routes, it's going to rerender the page or route that they're looking for regardless. But I guess whatever you feel.

Comment thread src/index.js
Comment on lines +229 to +232
if (onPrivateRoute && element.props.private) {
onPrivateRoute(props.navigate);
}

@airtonix airtonix Jun 6, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think you need to treat the function as if it would return a Boolean describing the bailout:

// peasant javascript
if(typeof onBeforeRender === 'function' && !onBeforeRender(props) ){
 return ''
}

Then the remaining render code below won't run, and won't cause flicker, or more importantly avoid fail states in the rendered component due to possible missing values that can be checked for in the above onBeforeRender

@airtonix

airtonix commented Jun 6, 2020

Copy link
Copy Markdown

At the moment we deal with this by wrapping the components we want to associate to a route with either of two components: PublicRoute and PrivateRoute.

They both take props:

  • component, which can be a function or a component.
  • redirectTo, some location when the checks fail.
  • the other standard props a reach-router route takes.

They both check :

  • mobx store for user.isAuthenticated
  • does desired location match redirect?

If the checks fail for the private route, it will :

  • redirectTo (usually a login path) with a next parameter set as the desired location.
  • render empty string to prevent children having to implement excessive authentication checking logic.

After loginSuccess the user is sent back to where they tried to go.

So any solution that tries to allow logic when the matched route is about to render, but before actual rendering, needs to also determine if rendering should continue based on a return value.

@ereio

ereio commented Aug 30, 2020

Copy link
Copy Markdown

vue-router has beforeEach(function) and have used it successfully to check auth state with several apps without flicker. Would love for this to be prop to the Router component :)

@liho98

liho98 commented Jul 27, 2021

Copy link
Copy Markdown

I don't foresee this router taking an active stance on how to handle authenticated vs non-authenticated routing. This is best solved in user-land as different cases require different setups. Thanks for the contribution though!

Why r u closing this issue, pls go to try vue router beforeEach, u will know why so many people moving to vue js & vue router as well as people request it.

@vertic4l

vertic4l commented Jul 27, 2021

Copy link
Copy Markdown

@liho98 VueJS is an overhyped framework which sucks in my opinion.

And you don't need a beforeEach method:

<Route exact={true} path={Routes.SecretPlace}>
        <AuthenticatedRoute
            loginUrl={Routes.Login}
        >
            <SecretPlace />
        </AuthenticatedRoute>
</Route>,

and AuthenticatedRoute looks something like that:

render() {
        if (!this.state.authDone) {
            return null;
        }

        if (!this.props.stores.userStore.isLoggedIn) {
            return <Redirect to={this.props.loginUrl} />;
        }

        return this.props.children;
    }

so AuthenticatedRoute can do what beforeEach method would do.

@liho98

liho98 commented Jul 27, 2021

Copy link
Copy Markdown

@liho98 VueJS is an overhyped framework which sucks in my opinion.

And you don't need a beforeEach method, just use your brain:

<Route exact={true} path={Routes.SecretPlace}>
        <AuthenticatedRoute
            loginUrl={Routes.Login}
        >
            <SecretPlace />
        </AuthenticatedRoute>
</Route>,

and AuthenticatedRoute looks something like that:

render() {
        if (!this.state.authDone) {
            return null;
        }

        if (!this.props.stores.userStore.isLoggedIn) {
            return <Redirect to={this.props.loginUrl} />;
        }

        return this.props.children;
    }

seems like u dont know how vue router beforeEach exactly works. Based on your example, if i have 100 routes, i need to wrapped the function 100 times, this is sucks!

@vertic4l

vertic4l commented Jul 27, 2021

Copy link
Copy Markdown

@liho98 Seems like you are just not experienced enough to find solutions on your own. :)

<Switch>
    <Route />
    <Route />
    [SecretPlace1, SecretPlace2, SecretPlace3].map(SecretPlace => <Route><AuthenticateRoute>{SecretPlace}</AuthenticateRoute></Route>)
</Switch>

@liho98

liho98 commented Jul 27, 2021

Copy link
Copy Markdown

@liho98 Seems like you are just not experienced enough to find solutions on your own. :)

// BAD
<Switch>
    <Route />
    <Route />
    [SecretPlace1, SecretPlace2, SecretPlace3].map(SecretPlace => <AuthenticatedRoute>{SecretPlace}</AuthenticatedRoute>)
</Switch>

lol, u failed to get what i mean exactly, based on this array example, same its wrapped the suck function 100 times. What i want is without wrapping the route, listen to an beforeEnter event or a beforeEnter callback just like how vue router works, handling it elegantly.

// GOOD
router.beforeEach((to, from, next) => {
  if (to.path !== '/login' && !isAuthenticated) next({ path: '/login' })
  else next()
})

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

Successfully merging this pull request may close these issues.

8 participants