Skip to content

Conversation

@mr-rob0to
Copy link
Contributor

@mr-rob0to mr-rob0to commented Feb 18, 2019

Summary of changes

Refactored the existing routes definitions and created two new HOC components to take advantage of React's Context API.

The new components are:

AuthenticatedRoute - this replaced the RouteWithUser component which was used to keep track of and pass the user object around to ensure only authenticated users accessed these routes and components. With the new AuthProvider sharing the context, this new component is able to consume and access the user object and pass it onto the target component for a particular route.

AuthProvider - this new component is responsible for creating the context for the application and exposing it to any consuming components. In addition, it provides two new functions that are used in other components to gain access (consume) to the context. This provider component is now set at the top level of our routes and components so all of the child components can "see" the context.

Today, the only context of any value that we're creating is the user object. We can certainly add more context values as we see fit in the future. Lastly, minor updates were made to existing components to take advantage of this new context API.

Overall, with this change, we now remove the need to pass the user object down to nested components who need it via props and offer a way for the application to be aware of the user's state across all components.

  - add auth provider at top level of routes
  - add authenticated route component
  - fix login page temporarily being displayed on home component refresh
  - add proper redirect on forgot-password component if user logged in
  - update navbar component to leverage context API
  - delete unused withUser function
  - delete unused route components
@mr-rob0to mr-rob0to requested a review from lpatmo February 19, 2019 04:19
@mr-rob0to
Copy link
Contributor Author

Cleaned up most of the remaining items! Let me know if you have any questions or concerns. Otherwise, I will be setting this draft PR to ready for review soon.

Copy link
Member

@gauravchl gauravchl left a comment

Choose a reason for hiding this comment

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

@d3vild06 This looks great! 👏

Few thoughts

  • For every non authenticated route(login, forgotPassword...) AuthContext is being used inside the component. Can we reduce this code duplication. I mean wrap this logic at route level something like:
<NonAuthRoute path="/login" component={Login} redirectIfAuth='/home'>
<NonAuthRoute path="/forgot-password" component={ForgotPassword} redirectIfAuth='/home'>

...

const NonAuthRoute = ({ component: Component, redirectIfAuth, ...rest }) => (
  <AuthConsumer>
    {({ isLoggedIn, user }) => (
      <Route
        render={props =>
          isLoggedIn
          ? <Redirect to={redirectIfAuth} /> 
          : <Component {...props} />
        }
        {...rest}
      />
    )}
  </AuthConsumer>
)
  • After hitting logout btn i redirected successfully to login page but navbar btn is still showing "Logout" button. You might wanna set user = null when Accounts.onLogout(_ => this.setState({ user: null }))

  • AuthProvider: In Meteor if user is not logged in Meteor.user() returns null otherwise it returns user object. Inside AuthProvider how about we use the same behavior instead using extra "isLoggedIn" state?

...
state = { user: Meteor.user(), isLoading: true }
...
componentDidMount() {
  // set isLoading = false
}
...
<AuthContext.Provider value={{ user: this.state.user }}>
...

@mr-rob0to mr-rob0to marked this pull request as ready for review April 16, 2019 21:40
@lpatmo
Copy link
Member

lpatmo commented Apr 16, 2019

Thanks @d3vild06! Do you need help resolving the merge conflicts?

  - add auth provider at top level of routes
  - add authenticated route component
  - fix login page temporarily being displayed on home component refresh
  - add proper redirect on forgot-password component if user logged in
  - update navbar component to leverage context API
  - delete unused withUser function
  - delete unused route components
@lpatmo
Copy link
Member

lpatmo commented Apr 23, 2019

Thanks for resolving those merge conflicts @d3vild06! Can you write up a quick summary of what you did? E.g. how the three higher-order components you introduced relate to the context API.

@lpatmo lpatmo merged commit b4fd439 into staging Apr 24, 2019
@angelocordon angelocordon deleted the refactor-auth-routes branch April 28, 2019 18:30
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.

4 participants