-
Notifications
You must be signed in to change notification settings - Fork 35
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
Migrating action creators to Redux saga #77
Conversation
@@ -21,7 +21,7 @@ export default class CollectionEdit extends Component { | |||
|
|||
render() { | |||
const {collection} = this.props; | |||
const {label, schema, uiSchema, displayFields, busy} = collection; | |||
const {schema={}, uiSchema={}, displayFields=[], label, busy} = collection; |
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.
wao, I didn't know about that syntax :) neat !
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.
Me neither 2 days ago :p Now I see so many places this should be used it's embarrassing.
This is now ready for, ahem, review. I'm pretty sure no one on Earth will ever want to thoroughly review this, so I'm gonna let it sit for a few hours and land it if I don't get any feedback meanwhile. Pinging @Natim @leplatrem @glasserc @almet |
I will have a look at it :) |
|
||
let client; | ||
|
||
export function setupClient({server, username, password}) { |
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 wonder if we shouldn't pass the authorization header directly to later support other authorization methods.
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.
yeah, but please let's iterate 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.
Ok 👍
There is tests 👍 |
I read the code, it looks good to me. |
I'm landing this. |
This patch leverages redux-saga and moved the action creators logic to dedicated sagas. Actions are now just simple synchronous descriptors, and sagas handle the asynchronous request stuff, triggering actions to notify the reducers when they're processed.
That makes the codebase better organized and easier to read especially because of the use of generators.
While the diff is large and seems to add more code than before, this is mostly because we now have much more granular tests for action creators.
The patch has been deployed to gh-pages.
Todo