-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat: add safe effect helper #552
Conversation
src/sagas/helpers.js
Outdated
*/ | ||
export function safeEffect(effect, onError) { | ||
// eslint-disable-next-line func-names | ||
return function* (payload) { |
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.
question(non-blocking): Why not name this function? Wouldn't it help in an eventual debugging session?
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'm giving it a name _safeEffect
: 611e63f. Thank you!
src/sagas/nanoContract.js
Outdated
*/ | ||
function* registerNanoContractOnError(error) { | ||
log.error('Unexpected error while registering Nano Contract.', error); | ||
yield put(nanoContractRegisterFailure(failureMessage.blueprintInfoFailure)); |
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.
question: Why use the blueprintInfo
failure when the error was on a registration event?
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.
Fixed: b405bbe
takeEvery(types.NANOCONTRACT_REGISTER_REQUEST, registerNanoContract), | ||
takeEvery( | ||
types.NANOCONTRACT_REGISTER_REQUEST, | ||
safeEffect(registerNanoContract, registerNanoContractOnError) |
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 solution! For the future, I'm thinking of a way to make it more generic, and not require two handler functions for each effect.
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.
It would be awesome!
2478b95
to
611e63f
Compare
Acceptance Criteria
Issue: #550
Note
This PR can be cherry-picked to refactor/default-network branch.
Security Checklist