-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Magic Login: Control flow state with redux instead of separate routes #12732
Conversation
6238b79
to
f11fd33
Compare
f11fd33
to
1694206
Compare
2665f70
to
9d8b038
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 tests pass and make sense. I also manually tested by applying D5031-code and everything works as expected. Good job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I wasn't around in time for pre-merge review, but here are my comments.
onClickEnterPasswordInstead: () => dispatch( hideRequestForm() ), | ||
onNoticeDismiss: () => dispatch( hideRequestNotice() ), | ||
onSubmit: emailAddress => dispatch( fetchRequestEmail( emailAddress ) ), | ||
setInputEmailAddress: emailAddress => dispatch( setInputEmailAddress( emailAddress ) ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of manually wrapping functions with dispatch, I recommend defining mapDispatch
as an object:
{
onClickEnterPasswordInstead: hideRequestForm,
…
}
It's shorter, less error-prone, and offloads mapper generation to Redux by being more declarative — allowing it to memoize if able to.
onMagicLoginRequestClick: event => { | ||
event.preventDefault(); | ||
dispatch( showMagicLoginRequestForm() ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should only use mapDispatch
to wrangle bindings of state, props and dispatch. The presence of event
here is a code smell to me: this a function that should be defined in the component. So:
const mapDispatch = {
showMagicLoginRequestForm
};
Then in the component class:
onMagicLoginRequestClick = ( event ) => {
event.preventDefault();
this.props.showMagicLoginRequestForm();
}
type: MAGIC_LOGIN_SET_INPUT_EMAIL_ADDRESS, | ||
} ); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for fetchRequestEmail
, all action creators here should be pure functions returning objects, and not thunks dispatching said objects.
requestAuthSuccess, | ||
requestEmailError, | ||
requestedEmailSuccessfully, | ||
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite a hefty set of reducers. We should chat about these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if not passed a schema
, createReducer
will by default set handlers for {,DE}SERIALIAZE
to return the reducer's initial state, meaning that for all of the above reducers we should get rid of those lines.
export const requestAuthStatus = state => get( state, 'login.magicLogin.requestAuthStatus', null ); | ||
|
||
export const emailAddressFormInput = state => get( state, 'login.magicLogin.emailAddressFormInput', '' ); | ||
export const emailAddressFormInputIsValid = state => get( state, 'login.magicLogin.emailAddressFormInputIsValid', false ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For new development, we're deprecating state/<foo>/selectors
in favor of https://github.com/Automattic/wp-calypso/blob/master/client/state/selectors/README.md. I forgot to mention this earlier.
This PR moves to redux actions to control state & converges on the
/login
route (instead of/login/send-me-a-link
, for example).Manual Testing
Email me a login link
linkAutomated Testing
npm run test-client -- --grep "state login"
For item
Initiate state changes via Redux actions
in #12677TODOs (in follow up PR(s))