-
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
Support User: Add Redux integration #2512
Conversation
1de7339
to
4df8b5c
Compare
I tested the functionality a bit and it's working as I'd expect. The code looks good to me, but I'd rather have more experienced eyes on it to thumbs up a merge. |
login: ( username, password ) => user.changeUser( username, password, callback ), | ||
logout: () => user.restoreUser() | ||
login: ( ...args ) => reduxStore.dispatch( supportUserFetchToken( ...args ) ), | ||
logout: ( ...args ) => reduxStore.dispatch( supportUserRestore( ...args ) ) |
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.
Any reason why you're hiding the function signature here?
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.
Or go crazy and skip the arguments…
const storeDispatch = reduxStore.dispatch.bind( reduxStore );
window.supportUser = {
login: compose( storeDispatch, supportUserFetchToken ),
logout: compose( storeDispatch, supportUserRestore )
}
😄
} | ||
callback( error ); |
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.
how about instead of merging this into a nested structure we just abort early
if ( error ) {
return callback( error );
}
this.once( 'change', callback ); // no need to create a closure for calling callback
this.clear();
this.fetch();
Looks pretty good @jordwest - sure would be nice to see some tests here to verify this! |
88204bb
to
0ea14b7
Compare
export function supportUserRestore() { | ||
let user = new User(); | ||
user.clear(); | ||
user.restoreUser(); |
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.
There's quite a few side-effects occurring in the functions of this file. The action creators should really only be returning simple objects (or in the case of thunks, dispatching objects as asynchronously). It seems that these are needed for interoperability between the old-style user store, though I'd argue that we should start to manage the behavior of the current user exclusively from client/state/current-user
.
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.
I second what @aduth is saying here, though withheld my comments previously because it seemed to be mostly stage one in moving the legacy code to Redux. But consider this a plus one, a bump, or a thumbs-up to what he said 😉
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.
Thirded. I was also hesitant writing these calls in the action creators, but at this stage I can't think of another way around it until User
and UserSettings
libraries are fully connected to the Redux store. That looks like a fairly sizeable task in itself and at the moment HEs are pretty keen to start using this feature; would you both be happy with this as is for a temporary solution and we can revisit later?
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.
That's fine with me, which is why I didn't originally bring it up. I'd rather have it here than in the store.
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.
There's another issue with these intermediary steps that affect the purity of the state — for part of our offline strategy we are dumping state into storage and hydrating on boot, we are also exploring queues of actions to be replayed later on. Anything that affects what's expected of a pure redux flow would add obstacles in the way. I'm of the mind that as long as we have coexisting data structures we need to duplicate code and handle it in both places, or fully rework the old code to the new state tree.
Result of
|
This change adds the Redux actions and reducer for the Support User UI in an upcoming PR.
7d8c4f7
to
b81de33
Compare
Thanks for the feedback @dllh, @gwwar, @mtias, @dmsnell, @aduth. Based on the discussion above, I went back to the drawing board and came up with a way to decouple the Redux store from the low-level parts, and re-implemented this PR in #2819. It removes any side-effects in the action creators, while a I'll close this PR for now as #2819 replaces it. |
Previous PR: #2368
This change adds the Redux actions and reducer for the Support User UI in an upcoming PR. There are no changes to functionality in this PR.
Ref: p195om-2GO-p2
cc @gwwar