-
Notifications
You must be signed in to change notification settings - Fork 60
Move the Login logic to middlewares - Closes #596 #603
Conversation
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.
Looks good 👍
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.
Nice. The code looks some much more organized with middlewares :-)
Just the next
vs dispatch
issue.
src/store/middlewares/login.js
Outdated
return getAccount(activePeer, address).then(accountData => | ||
getDelegate(activePeer, publicKey) | ||
.then((delegateData) => { | ||
next(accountLoggedIn(Object.assign({}, accountData, accountBasics, |
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 think it would be better to call store.dispatch(...)
than next(...)
. RIght now it's not a big difference, but once we have more middlewares it will be easier if each action goes through each middleware.
https://stackoverflow.com/questions/40444158/should-you-dispatch-or-call-next-multiple-times-inside-of-a-redux-middleware
src/store/middlewares/login.js
Outdated
next(accountLoggedIn(Object.assign({}, accountData, accountBasics, | ||
{ delegate: delegateData.delegate, isDelegate: true }))); | ||
}).catch(() => { | ||
next(accountLoggedIn(Object.assign({}, accountData, accountBasics, |
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.
Same here
src/store/middlewares/login.test.js
Outdated
middleware(store)(next)(activePeerSetAction); | ||
expect(next).to.have.been.calledWith(); | ||
|
||
accountApiMock.restore(); |
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 ok for now.
But for the future, I just discovered a more fault-tolerant and less verbose way to restore stubs, mocks, etc. - sinon-test
https://www.npmjs.com/package/sinon-test
Close #596