Best practice on handling data flow for login / signup pages with redirect #297
Comments
|
Instead of having Also, resetting the state could be done by just processing the Does this make sense? :D |
|
It makes sense if there is just the login page, but it's a bit more generic approach. For example for a signup page, or password reset page. Even if you are logged in you should still be able to see those pages. And those flags should be used by all those pages, because they all have the same flow. Hope it's more clear now :) |
|
Yeah, from my point of view "shouldRedirect" is just too generic in order to be associated with a certain action. It might open up bugs where you somehow forget to set that flag and then a page redirects to another page to which it actually shouldn't redirect. But sure, if you want to clean your state, you can just do that with an action. Or you could listen to a history change outside of react and then trigger the state-clearing action. That might be preferable because you don't have to scatter that logic throughout your components but you can keep it in a single place. But that is only a possible solution for the state clearing. I'm not sure yet how to solve the redirection in a nice way. |
|
The question is also: does it make sense to have those flags in the state at all? If not, how should this flow be implemented? |
|
@gaearon if you have some time I'd love to hear your feedback as well. Thanks! 👍 |
|
In my application I am redirecting in middleware and I don't use any flags. But I am not using react-router. // action
/**
* Creates login action
*
* @param {string} email
* @param {string} password
*
* @returns {{types: *[], promise: *}}
*/
export function login(email, password) {
return {
types: [LOGIN_REQUEST, LOGIN_SUCCESS, LOGIN_FAILURE],
promise: post('/login').send({ email: email, password: password }).promise()
};
}
// auth middleware
/**
* Intercepts LOGIN action and redirects to login screen
* Otherwise just sends action to next middleware
*
* @returns {Function}
*/
function authMiddleware({getState, dispatch}) {
return (next) => (action) => {
if (typeof action === 'object' && action.hasOwnProperty('type')) {
if (action.type === LOGIN_SUCCESS) {
next(action); // send it to next so identity will be set
// get current route
const state = getState();
let path = '/dashboard';
if (typeof state['router'] === 'object' && typeof state['router']['route'] === 'object' && null !== state['router']['route']) {
if (state.router.route.name === 'login' && typeof state.router.route.query['to'] === 'string') {
path = state.router.route.query.to;
}
}
return next(actions.transitionTo(path));
}
}
return next(action);
};
}It is not refactored, it is inteded only for test purposes of my router solution :). |
|
This is a copy and paste from my application and how I handle loading user data and logging in. I'll try to comment the code as best as possible, and I'll be on reactiflux for questions. const AppConnector = createConnector((props$, state$, dispatch$, context$) => {
// true when the user is logged in - drawn from Store state
const loggedIn$ = state$
.map(s => s.apiKey !== null)
.distinctUntilChanged()
// this returns an observable of a path to redirect to
// in my app, I have a url like `/login?nextPath=dashboard`, but if that
// isn't set, just go home
const redirectTo$ = routeState$
.map(s => s.router.query.nextPath || '/home')
const redirect$ = loggedIn$
.withLatestFrom(
context$,
redirectTo$,
(loggedIn, context, path) => () => context.router.transitionTo(loggedIn ? path : '/login') // when the loggedIn state changes, i.e. user has logged in or out, respond to that change
)
.do(go => go())
// It's awesome to have this here, because it means that no matter
// where the user loads in to the app (all child views of this view),
// the app will realise it needs to load data, and loads the according data
const userData$ = state$
.map(getUser)
const userDataIsEmpty$ = userData$
.map(user => user.id === null || typeof user.id === 'undefined')
.filter(s => s === true)
const loadUserData$ = Rx.Observable.combineLatest(loggedIn$, userDataIsEmpty$, (logged, userDataIsEmpty) => loggedIn && userDataIsEmpty)
// only when user is logged in but has no user data
.filter(s => s === true)
// fetch data with an action creator
.withLatestFrom(dispatch$, (userData, dispatch) => () => dispatch(UserActions.loadUserData()))
.forEach(go => go())
return Rx.Observable.combineLatest(props$, state$, context$, redirect$, (props, state, context) => ({...props, ...state, ...context}))
}, render) |
|
In terms of answering your questions @emmenko, I think having |
I agree. Things that are only useful once shouldn't be in the state IMO. |
|
Also, as per slack, I don’t think you should be storing See https://github.com/faassen/reselect for more selector usage |
|
@frederickfogerty thanks for the example. Question though: this works fine for login because you usually have a
@gaearon I agree, but I'm still not sure what's the correct approach for that. You still need to have a state somewhere... This is how I'm doing it atm without storing anything in the state. // actions
function login () {
return { type: 'LOGGED_IN' }
}
function submitLogin (form, dispatch) {
return api.doLogin(form)
.then(() => {
dispatch(login()) // can dispatch something at this point
return Promise.resolve()
})
}
// in the component (login, signup, ...)
onSubmit () {
actions.submitLogin(form, dispatch) // this returns a promise
.then(() => this.setState({ shouldRedirect: true }))
.catch(error => this.setState({ shouldRedirect: false, errorMessage: error }))
}
Yep, makes sense. |
|
@gaearon I guess the question for this general workflow (redirect or show error) is if it should be done with "redux" or in a different way. And what would be this alternative? |
|
related and I will pin it here... actually I was also dealing with this problem and currently trying to reimplement following example into Redux. Nothing showable yet because I'm doing this in my free time: It is pure Flux: Maybe it is useful for best practices?! |
As long as your using is logging in, then The best approach I've taken is to add this logic to a connector where the functionality is needed for its children e.g. for authorisation, say you have a
I definitely don't like that approach, that has two way data flow between the UI and the ACs, which goes against flux. If you're tied to doing it imperatively, you could do something like
This works for all the pages you mentioned |
|
Sorry but I still don't see how I handle the redirect / error rendering. Let's take a step back and say I have the This is what I would like to know, how the flow should be and where should it live. PS: thanks for your feedback so far! |
This is nowhere near as elegant as for the login and sign up pages, as it is very coupled |
|
I have to sleep, I'll answer any more questions tomorrow |
|
So we are storing the flags in the state again ;) PS: good night! |
|
I think the question is a little bit deeper then just generalising 'submit forms' – it is more about what actually the state is and which data should appear in I like to think about class Form extends React.Component {
onFieldChanged (event) {
this.setState({[event.target.name]: event.target.value})
}
render () {
return (
<form onChange={::this.onFieldChanged}>
<input type="text" name="name" />
<input type="text" name="surname" />
<input type="email" name="email" />
</form>
)
}
}During typing into this fields some internal state modifications performed so we will have something like this at the end: {
name: 'John'
surname: 'Snow'
}So we got some state which doesn't linked to the application state and nobody cares about it. When we will revisit this form next time it will be shown erased and the 'John Snow' will be gone forever. Lets assume that this is subscription form for some notification letter. Now we should interact with outer world and we'll do it using our special action. import { subscribe } from 'actions'
class Form extends React.Component {
onFieldChanged (event) {
this.setState({[event.target.name]: event.target.value})
}
onSubmit (event) {
event.preventDefault()
const { name, surname, email } = this.state
subscribe(name, surname, email)
}
render () {
return (
<form onChange={::this.onFieldChanged} onSubmit={::this.onSubmit}>
<input type="text" name="name" />
<input type="text" name="surname" />
<input type="email" name="email" />
<button type="submit">Subscribe</button>
</form>
)
}
}When user pushes submit button he expect any response from the UI was it succeed of not and what to do next. So we want to keep track the request state to render loading screen, redirect or show error message. render () {
//provided by connector of whatever
const { props: { isLoading, error } } = this
return (
<form onChange={::this.onFieldChanged} onSubmit={::this.onSubmit}
disabled={isLoading}>
<input type="text" name="name" />
<input type="text" name="surname" />
<input type="email" name="email" />
<button type="submit">Subscribe</button>
{error ? <p>{ error }</p> : null}
</form>
)
}But here the devil appears: now we have this page state persisted and next Does But in case of pure submit forms we could assume that the form state has nothing to do with application state: we want to know the fact is user So how about proposition we don't need this state somewhere else except Component? Lets give it a try. function subscribe (name, surname, email) {
return api.request('/subscribtions', 'create', { name, surname, email })
}At next step we could start keeping track of the action in our onSubmit (event) {
event.preventDefault()
const { name, surname, email } = this.state
subscribe(name, surname, email)
.then(() => { this.setState({ submitted: true }) })
.catch(error => { this.setState({ error }) })
}
componentWillUpdate (object nextProps, object nextState) {
if(nextState.submitted)
redirect('somewhere')
}Looks like we've got everything we need to render component without polluting application state and got rid of need to clean it. import React, { PropTypes } from 'react'
import { bindActionCreators } from 'redux'
// Keeps track of action
export default function connectSubmitForm (Form, submitAction) {
return React.createClass({
contextTypes: {
//redux Store
store: PropTypes.object.isRequired
},
getInitialState () {
return {}
},
onSubmit (...args) {
const { context: { store: { dispatch } } } = this
const { submitAction: submit }
= bindActionCreators({ submitAction }, dispatch)
submit(...args)
.then(() => this.setState({ submitted: true }))
.catch(error => this.setState({ error }))
},
render () {
const {
onSubmit,
props,
state: { submitted, error }
} = this
return (<Form {...props} onSubmit={onSubmit} submitted={submitted}
error={error} />)
}
})
}and // redirect to path if predicate returns true
export default function redirect (path, predicate) {
return Component =>
class Composed extends React.Component {
componentWillMount () {
if (predicate(props))
redirectTo(path)
}
componentWillReceiveProps (nextProps) {
if (predicate(nextProps))
redirectTo(path)
}
render () {
return <Component {...this.props} />
}
}
}
//redirect to path if submitted
export default function redirectSubmitted (path) {
return redirect(path, ({ submitted }) => submitted)
}Now on one hand we have @redirectSubmitted('/')
class Form extends React.Component {
onFieldChanged (event) {
this.setState({[event.target.name]: event.target.value})
}
onSubmit (event) {
event.preventDefault()
const { name, surname, email } = this.state
this.props.onSubmit(name, surname, email)
}
render () {
return (
<form onChange={::this.onFieldChanged} onSubmit={::this.onSubmit}>
<input type="text" name="name" />
<input type="text" name="surname" />
<input type="email" name="email" />
<button type="submit">Subscribe</button>
{this.props.error ? <p>{ this.props.error }</p>: null}
</form>
)
}
}
export default submitForm(Form, subscribe)And here it is: pure render form with no dependencies of Thanks for reading and sorry for taking your time. |
|
Fair enough! To be honest I'm still interested in looking at storing local component state in Redux: #159. |
|
@stremlenye That is pretty slick. Thank you for sharing! |
|
Closing—when RR 1.0 comes out, we'll cover this in “Usage with React Router” recipe. |
|
@stremlenye your approach is pretty awesome! Would it make sense to - instead of using a decorator - use a react-router higher order component in which all the unathorized ones would be nested and it would be responsible for redirecting once the auth store would have a user in? Something like this, where the |
|
@tomazzaman Thanks :) |
|
I've been wrapping my head around this too, and it seems @stremlenye did a great job showing how it can be done. The problem is that one tends to get stuck in the "everything needs to go through the action - reducer - update-component-with-store-state loop" (me too) and that we then tend to pollute the store with all kinds of state that's only used in very specific scenarios in the app (on 1 screen, stuff such as "did I succesfully execute the reset password"), whilst this can be easily handled by just doing the async calls inside the component and updating the component itself using Will try adapting my code here too to see if it cleans things up. |
|
@ir-fuel if will have time could you push a small example of what you will get to the github? |
|
@sompylasar @kpaxqin I think @ir-fuel suggested the right approach – if you need something intermediate to be eventually persisted, persist it on the client side: much easier to implement, doesn't really involve server into persisting inconsistent data, doesn't perform any http requests (what is also a major problem on mobile devices) and extremely easy to change the approach in future. And about persisting any data somewhere – think how you will clean it after process failed or done already – that was the initial theme of a topic. |
|
It's not so much "store it client or server side", it's more a matter of "do I handle this 'ad hoc' in my parent component class" vs "do I do the whole action/reducer/state dance". |
|
@ir-fuel 👍 |
|
@stremlenye Please read my answer again. I say explicitly that you don't have to make a server-side API to make a persistence API layer. @ir-fuel Not going through actions/reducer cancels predictability and replayability of the state transitions. |
|
@ir-fuel You can easily clear your app state after the form is submitted, so it does not get "pollutted". |
|
RE: clearing. Make an action CLEAR and have a button in your UI to reset the sign-up flow. You can make it via router, too (dispatch that action when visiting the initial signup URL, and if the signup has started, change the URL to something else). |
|
@sompylasar there is no problem to reset your state with an explicit button – problems occur after unhandled failures: it is hard to predict the ways state could be corrupted and define a restore strategy for each particular case. |
|
@stremlenye yes, software engineering is hard sometimes. Predictable states and transitions help to mitigate this. |
|
Not only that, but if your state is more than a bunch of json objects (which I hate because everything is by convention and nothing is enforced) and you use something in the line of Immutable.js' For me building something with redux without using Immutable.js is just an accident waiting to happen. I know that you could make a |
For now, I'm not convinced on using Immutable.js with redux, it appeared too complex to integrate (and not performant). A deep-freeze during development and testing, and a set of tests should be enough to cover most of the cases. And a strict convention to avoid mutating state in reducers or other places where you have the reference.
Yes, that's what I had in mind.
That's what we call explicit application state -- in every moment in time we know the state of the app which consists of many components. If you want to make the state structure less explicit and more dynamic, follow https://github.com/tonyhb/redux-ui |
|
That's the whole problem with JS development. "Let's stick to conventions". And that's why people start to realise that it just doesn't work in complex environments, because people make mistakes, and come with solutions such as TypeScript and Immutable.js. Why would it be too complicated to integrate? If you use 'Record' you use all those objects (as long as you are just reading them) as normal JS objects, with dot notation to access the properties, with as an advantage that you declare up front which properties are part of your object. I am doing all my redux development with Immutable.js, with no problems whatsoever. The fact that simply cannot by accident modify an object is a big advantage in redux and the 'type safety' of defining your object prototypes is convenient too. |
@ir-fuel
Maybe I haven't describe the fake API clear enough, the most important reason to build it is I don't want to put those things //action
submitFormA (formA) {
fakeApi
.saveFormA(formA)
.then(()=>{ transitionTo(nextPage) })
.catch()
}and in FormPage_C ( the final form ): submitFormC (formC) {
fakeApi
.getFormAnB()
.then(mergeFormData(formC)) //mergeFormData is a curry function
.then(realApi.submitSignUp)
.catch()
}All those temporary data lives in fake api and the global application state haven't been polluted. If we want to clear data in fake api to restart flow, just send action to do it.
@stremlenye I agree with that, But I think those particular failure cases are more likely to be business things, they are always here no matter where we put the state, and they should be handled in right way but just not been described clear enough at the moment. The only thing devs can do is make it easy to track so that we can fix it with less pain. And put state in parent component might make it harder to reason about when things go complex. With fake api we mutate that state with actions and it seems more predictable. But I agree that the fake API looks too heavy in most cases with less complexity. |
|
Didn't go through all messages above. Here I just give my approach: detect prop changes in componentWillReceiveProps. Say that the state shape is: { loginPending, loginError }, when begin login, set loginPending = true. When success or failed, set { loginPending: false, loginError: 'some error.' }. Then I can detect if login success event in a component and redirect the page: It just works though not beautiful. |
|
My humble implementation regarding clearing the form on redirection using const initialState = {
login: initialLoginState,
signup: initialSignupState,
// ...
};
export default function(state = initialState, action) {
switch (action.type) {
case '@@router/LOCATION_CHANGE':
return {
...state,
login: initialLoginState,
signup: initialSignupState
};
// ...Do you think that this approach is OK? It works. |
|
I'm ended up with this. Tell what you think about: register redux action: import { checkStatus } from 'helpers/fetch_helpers';
import { AUTH } from 'config/constants.endpoints';
import { USER_LOGGED_IN, USER_LOGGED_OUT, USER_REGISTERED } from 'config/constants.actions';
import { replace } from 'react-router-redux';
// other actions...
const userRegistered = (user) => ({
type: USER_REGISTERED,
user
});
export const register = (formData, redirectTo = '/') => {
const fetchParams = {
method: 'post',
body: formData
};
const handleData = (dispatch) => (response) => response.json().then( (data) => {
dispatch(userRegistered(data));
dispatch(replace(redirectTo));
});
return (dispatch) => fetch(AUTH, fetchParams)
.then(checkStatus)
.then(handleData(dispatch));
};register form container: import React, { Component } from 'react';
import RegisterForm from './register_form';
import { bindActionCreators } from 'redux';
import { connect } from 'react-redux';
import * as userActions from 'store/actions/user';
import { parseFormErrors } from 'helpers/fetch_helpers';
class RegisterFormContainer extends Component {
state = {
errors: {}
}
async handleSubmit(e) {
e.preventDefault();
const form = e.currentTarget;
const formData = new FormData(form);
const { register } = this.props.actions;
try {
await register(formData);
} catch (error) {
const errors = await parseFormErrors(error);
this.setState({ errors });
}
}
render() {
return (
<RegisterForm handleSubmit={ ::this.handleSubmit } errors={ this.state.errors } />
);
}
}
const mapDispatchToProps = (dispatch, ownProps) => ({
actions: bindActionCreators({ register: userActions.register }, dispatch)
});
export default connect(
null,
mapDispatchToProps
)(RegisterFormContainer);and helpers: export const checkStatus = (response) => {
if (response.ok) {
return response;
} else {
const error = new Error(response.statusText);
error.response = response;
throw error;
}
};
export const parseFormErrors = (error) => error.response.json().then( (data) => {
if (data.errors) {
const joinedErrors = _.mapValues(data.errors, (errors) => errors.join(' ') );
return { ...joinedErrors };
} else {
console.error('request failed:', error);
return {};
}
});What is important - only successful registration goes into the Redux store as action, otherwise error passes to the container component state. User getting an error, then navigates to another page, then again to the form page - form is empty and no errors. There is no "USER_REGISTRATION_REQUEST" or "USER_REGISTRATION_FAILURE" actions because they are describing component state, not application state. |
|
You've sugared your code with |
To achieve that, you can dispatch an action like |
|
thanks for feedback.
these conditions exist regardless of whether I use the async/await or not.
as intended, that is component state.. why this should be in app state?
in progress...
as intended
that is not true. user will be registered and redirected if request will be successful, regardless of the component state or even existence. look at
That is the point. Another action? Why so much boilerplate? Disable or enable submit button, show form errors or live validation, show spinner while request is open.. and these actions lead to new actions that restore the default state.. why this is have to be in app state? |
Yes, but this leaves the possibility of two subsequent
For predictability and replayability, this is the purpose of Redux. Having only the actions and the initial state, you can get the app into a certain state by only dispatching actions. If a component starts to contain some state, the app state starts to break apart, there becomes no single state tree that represents the app state. |
|
Don't be overly dogmatic about it, though. There's plenty of reasons to put stuff into Redux state, and plenty of reasons to leave something in local component state, depending on your scenario. See http://redux.js.org/docs/FAQ.html#organizing-state-only-redux-state and https://news.ycombinator.com/item?id=11890229. |
|
Ok, now it clear for me: it depends. Thanks! |
|
I use Is there anything wrong with doing it this way? |
|
What about such approach http://codereview.stackexchange.com/questions/138296/implementing-redirect-in-redux-middleware? |
and others
EDIT: what I'm trying to understand here is how to handle data flow when you have a page with a form. After you submit the form you either redirect to another page or render an error message.
The challenge is where this information should be temporary stored in the data flow. Because after you redirect or render the message, the state (or information you stored) is not relevant anymore and should be reset or cleaned.
In a backend web application you usually have pages like login, signup, reset password, etc.
When the user navigates to the app and is not logged in, he gets redirected to login page. This can be easily done with RR
onEnterhook.Then the user will fill up the form and submit it. At this point usually if the request was successful you want to redirect to application root or to some other "secured" page. If it failed, you want to stay on the page and show an error message.
(This applies not only for login page, but other pages as well as mention before. I will just focus on the login for now)
So far you have this flow (which is common use case):
//login/(or some other page)If we implement this flow with redux, I guess I have following setup:
Given this flow is correct - hopefully :) - you can see that I have 2 flags in the application state:
shouldRedirectanderrorMessage.Those flags are only used when submitting the form.
First question: is that ok to have those flags in the application state?
Another thing to consider is that I have to "reset" those flags when I redirect because if I want to go to another page (e.g. signup, ...) I should have a clean state (
{ shouldRedirect: false, errorMessage: null }).So I might want to dispatch another action like
Have anybody had this problem? Am I missing something or is that the "correct" way to do it?
Are there any other alternatives?
Any feedback is very welcomed! :)
The text was updated successfully, but these errors were encountered: