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

Auth page #41

Merged
merged 9 commits into from Feb 18, 2019

Conversation

Projects
None yet
2 participants
@Ofir-Smuha
Copy link
Owner

Ofir-Smuha commented Jan 29, 2019

  • Authenticate after redirect from Github

@Ofir-Smuha Ofir-Smuha requested a review from morsdyce Jan 29, 2019

}

componentDidUpdate() {
if (this.props.badCodeRequest) {

This comment has been minimized.

@morsdyce

morsdyce Jan 29, 2019

Collaborator

bad code request means the token was failed because of invalid code?
I think it will be better as a middleware flow

This comment has been minimized.

@Ofir-Smuha

Ofir-Smuha Jan 30, 2019

Author Owner

The component sends the received code and gets a token back, if the token is invalid, it updates the "badCode" to true, AuthenticatePage listen to updates and will get either token to badCode and will redirect to a different page accordingly. what do you mean by a middleware flow?

This comment has been minimized.

@morsdyce

morsdyce Jan 30, 2019

Collaborator

Consider this flow:

  1. component mounts and reads code from url
  2. component fires completeAuth api action with the code
  3. api action onError fires a navigate action
  4. Middleware catches navigate action and uses history api to navigate back to the login page and possibly showing a notification for the user.

I'd rather avoid counting on componentDidUpdate to get the prop which marks it as failed, it can run multiple times, we need to remember to clean it and we're left with data that isn't relevant to the application saved in our stores.

It's much easier to either create a specific middleware that will handle this (for example the middleware could also fire on itself the api action and handle the error itself).

This way we don't have to count on the componentDidUpdate method to run with new props and then decide we want to do something.
In general using componentDidUpdate without comparing the previous value (that it changed values) is very dangerous since it has the potential to cause an infinite loop

This comment has been minimized.

@Ofir-Smuha

Ofir-Smuha Jan 30, 2019

Author Owner

Perfect, much better. I changed it.

return next(action);
}

history.replace('/login');

This comment has been minimized.

@morsdyce

morsdyce Feb 6, 2019

Collaborator

this should be more generic. Think about creating specific navigate actions that you can tell where you want to go.

for example this action:
navigateTo('/login') which returns the following action { type: 'NAVIGATE', payload: { url } }

then the middleware will read this from the action.payload.url and navigate to where you need.

This way in the future if you want to navigate after some sort of action, or from a component click handler (when you can't use ) it will be very easy to do.

This comment has been minimized.

@Ofir-Smuha

Ofir-Smuha Feb 7, 2019

Author Owner

💯 , changed.

@morsdyce morsdyce merged commit da0f190 into release_candidate Feb 18, 2019

@morsdyce morsdyce deleted the auth-page branch Feb 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment