bigardone / phoenix-trello

Trello tribute done in Elixir, Phoenix Framework, React and Redux.
https://phoenix-trello.herokuapp.com
MIT License
2.5k stars 409 forks source link

Current user enanchement #28

Open alex88 opened 8 years ago

alex88 commented 8 years ago

Currently the logic to fetch the current user is this:

my idea is that it would be nice to fetch the user data before doing any consequent request or route entering, so that if we're in a non-authenticated page we can still have the header populated with the current user and in case we're in an authenticated page the route is loaded only after the user object is loaded in case the page needs some data inside the user object to do some logic. If for some reason the authentication fails it redirects to sign-in page.

What you think?

bigardone commented 8 years ago

Hi @alex88 :) Yeah, I totally agree with you. This project was my first attempt using this stack, and the way I did this part never convinced me totally. In other projects I've done recently, I've changed the authentication mechanism making it simpler. I need to find some time to refactor it, thanks for the suggestion :)

alex88 commented 8 years ago

My current setup is this one

export default function configRoutes(store) {
  const reloadAuthentication = (nextState, replace, callback, loginOnFail = false) => {
    const { dispatch } = store;
    const { session } = store.getState();
    const { currentUser } = session;

    // If there is on user loaded but there's a token we need to reload the user object
    if (!currentUser && localStorage.getItem('phoenixAuthToken')) {
      dispatch(SessionActions.reloadUser()).then(() => {
        callback();
      }).catch(() => {
        // If user reloading fails, we have to go to signin page
        replace('/sign_in');
        callback();
      });
    // If there is no token we have to redirect only if the page requires authentication
    } else if (!localStorage.getItem('phoenixAuthToken') && loginOnFail) {
      replace('/sign_in');
      callback();
    // In any other case, just go to the final route
    } else {
      callback();
    }
  };

  const ensureAuthenticated = (nextState, replace, callback) => {
    reloadAuthentication(nextState, replace, callback, true);
  };

  return (
    <Route component={MainLayout} onEnter={reloadAuthentication}>
      <Route path="/sign_in" component={SessionsNew} />

      <Route path="/" component={Home} />
      <Route path="/timeline" component={Timeline} onEnter={ensureAuthenticated} />
      <Route path="/creator" component={Creator} onEnter={ensureAuthenticated} />
    </Route>
  );
}

however I think it can be vastly improved

bigardone commented 8 years ago

It looks excellent! I did something similar on my Toggl clone project: https://github.com/bigardone/phoenix-toggl/blob/master/web/static/js/routes/index.js

export default function configRoutes(store) {
  const _ensureAuthenticated = (nextState, replace, callback) => {
  const { dispatch } = store;
  const { session } = store.getState();
  const { currentUser } = session;

    if (!currentUser && localStorage.getItem('phoenixAuthToken')) {
      dispatch(Actions.currentUser());
    } else if (!localStorage.getItem('phoenixAuthToken')) {
      replace('/sign_in');
    }

    callback();
  };

  return (
    <Route component={MainLayout}>
      <Route path="/sign_up" component={RegistrationsNewView} />
      <Route path="/sign_in" component={SessionsNewView} />
      <Route path="/" component={AuthenticatedContainer} onEnter={_ensureAuthenticated}>
        <IndexRoute component={HomeIndexView} />
        <Route path="/reports" component={ReportsIndexView} />
      </Route>
    </Route>
  );
}

The main difference is that I create a parent AuthenticatedContainer so I don't need to add the onEnter callback (call me lazy :P) on every private route child. What do you think about this solution?

alex88 commented 8 years ago

I wanted to have a common container to have the authenticated logic too but I'll implement that later. Anyway, what I experienced in my app is that sometimes one of the authenticated routes requires something in the currentUser object (in my case it was the socket, do you handle that case?

In my project I had to move the callback to where the setCurrentUser resolves so any route after that has the element already there

bigardone commented 8 years ago

Yeah! I store the socket in the store using the session reducer:

const initialState = {
  currentUser: null,
  socket: null,
  channel: null,
  error: null,
};

export default function reducer(state = initialState, action = {}) {
  switch (action.type) {
    case Constants.CURRENT_USER:
      return { ...state, currentUser: action.currentUser, socket: action.socket, channel: action.channel, error: null };

   default:
      return state;
  }
}

Action.currentUser() dispatches that CURRENT_USER action once the it connects to the socket and joins the user's channel https://github.com/bigardone/phoenix-toggl/blob/master/web/static/js/actions/sessions.js

alex88 commented 8 years ago

But, how can you be 100% that the socker connection end channel joining is done before the route has entered in the authenticated route?

If the route component during the didMount tries to use the socket after the user has done a full page reload, is the socket already connected and the channel already joined? In my case it wasn't and so the socket was still null

bigardone commented 8 years ago

Yeah, you're right. The only way I found to make it work using the store was like this:

https://github.com/bigardone/phoenix-toggl/blob/master/web/static/js/views/reports/index.js#L19

I basically use both componentDidMount and componentWillReceiveProps functions.

alex88 commented 8 years ago

Oh that's nice, so everything that needs the user object to be populated is started only after the first time the user is actually set by the authentication. I have never seen before that willReceiveProps.

This can solve my issue of having a blank screen until the user socket is connected and/or the current user object being set.

bigardone commented 8 years ago

Check this

https://github.com/bigardone/phoenix-toggl/blob/master/web/static/js/containers/authenticated.js#L24

If the current user is not set, it returns nothing on the authenticated container. When the user is set after authenticating it will render everything.

It's kind of tricky, but it works :)

alex88 commented 8 years ago

I think it would be nice to have the UI rendered instead of a blank page even if the user object is not set yet, that way you can show some page, start the loading spinning icon and then when you have the user object fetch the data and then do the requests

In any other case, when you just need the user authenticated, you can go straight with the localstorage token which should be enough to do an authenticated request

ACPK commented 7 years ago

@alex88 - Any chance you might be interested in doing a PR with your change? I'd love to check it out as I'm extremely interested in Phenix and Elixir.

alex88 commented 7 years ago

@ACPK thank you for the interest, unfortunately I'm not using react anymore and this project has ben re-done using another framework.

Anyway, the code that I was using was exactly this one https://github.com/bigardone/phoenix-trello/issues/28#issuecomment-221785539

If you need help with it just ask!

ACPK commented 7 years ago

@alex88 - What framework was the replacement built with?

alex88 commented 7 years ago

@ACPK angular 2, just because I was more familiar with it and I had to deliver the project in a small timeframe