Refactor user actions #246
Conversation
lib/actions/user.js
Outdated
| * - apiKey and apiBaseUrl from state.otp.config.persistence.otp_middleware, | ||
| * - accessToken and loggedIUnUser from state.user. | ||
| * - checks that otp_middleware is set, and throws an error if not. | ||
| * @param functionToExecute the code to execute, with parameters (dispatch, arguments) |
There was a problem hiding this comment.
Please note that functionToExecute can be a function that can be awaited upon.
evansiroky
left a comment
evansiroky
left a comment
There was a problem hiding this comment.
Just one comment about a comment
landonreed
left a comment
landonreed
left a comment
There was a problem hiding this comment.
This approach is far too clever. I really disagree with how the executeWithMiddleware method body is structured and how it is invoked throughout user.js.
It highlights the core logic for these actions and hides repeat code to extract and check common variables. |
evansiroky
left a comment
evansiroky
left a comment
There was a problem hiding this comment.
I'm going to have to change my review to request changes. I kind of agree with Landon in that this introduces a bit too much abstraction/overhead. I think the executeWithMiddleware shouldn't exist and instead it should simply be a util function that makes sure the middleware config exists and if so returns the desired middleware data given the getState method.
|
🎉 This PR is included in version 1.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR extracts the code to parse middleware configuration vars,
and inlines methods previously in
utils/middleware, as brought up by #224 (comment) and #224 (comment).