Issue #14 - Replace db transaction used when saving flattrs with a redux saga #15
Conversation
…h a redux saga
| @@ -39,7 +39,7 @@ function addAttention(tabId, url, addedAttention, isManual) | |||
|
|
|||
| if (addedAttention >= getRemainingAttention(entity, oldAttention)) | |||
| { | |||
| yield flattrManager.submit({ | |||
| flattrManager.submit({ | |||
There was a problem hiding this comment.
We're no longer waiting for the submission to succeed before continuing with the next steps. Why wouldn't this be an issue?
At least I could imagine that this leads to inconsistencies in the UI (e.g. attention bar filling up but flattr count not increasing).
There was a problem hiding this comment.
At least I could imagine that this leads to inconsistencies in the UI (e.g. attention bar filling up but flattr count not increasing).
At the moment if there is an error then nothing in the UI changes, is that better? I don't think it is, I think it's worse because we're just hiding errors from the user.
Also we reduce the chances of an error here because we make sure that only one save is performed at a time, and so we don't depend on transactions (which can cause errors).
Finally we at least have the chance to correct the error here, with logic that can detect error states like the one that you pointed out above (Pretty sure I didn't do this here yet, but I think that would be a good follow up issue).
| continue; | ||
| } | ||
|
|
||
| yield call(saveFlattrs, {}); |
There was a problem hiding this comment.
Detail: saveFlattrs() doesn't expect any arguments.
|
|
||
| for (let {entity, tabId, title, type, url} of saving) | ||
| { | ||
| let entry = yield call(save, {entity, tabId, title, type, url}); |
There was a problem hiding this comment.
What if save() fails? We do have SUBMIT_FLATTRS_FAILURE as well as SAVE_TIMESTAMP_FAILURE for the other cases for which we use Redux.
| }; | ||
| exports.flattrs = flattrsReducer; | ||
|
|
||
| const getFlattrs = (state) => ((state || {}).flattrs || {}); |
There was a problem hiding this comment.
Suggestion: I'd suggest writing this as (state = {}) => (state.flattrs || {}) to make this part a bit easier to read by avoiding the inner brackets.
| @@ -0,0 +1,100 @@ | |||
| "use strict"; | |||
There was a problem hiding this comment.
Suggestion: I noticed that this is almost an exact copy of test/tests/reducer-flattrs-submit.js so what about adding an abstraction so that we don't have so much duplicated code lying around?
| { | ||
| return new Promise((resolve) => | ||
| { | ||
| setTimeout(resolve, 0); |
There was a problem hiding this comment.
Suggestion: I assume you'd like to use setImmediate() here instead.
There was a problem hiding this comment.
I'm not sure why I would want to resolve a promise multiple times. It does look like I can get away with simply using Promise.resolve() here instead.
| assert.deepEqual(state.flattrs.save.saving, flattrsOne); | ||
|
|
||
| yield saga.waitFor(SAVE_FLATTRS_SUCCESS, true); | ||
|
|
There was a problem hiding this comment.
Detail: Please add an intermediate assert step here to avoid mixing together the effects of SAVE_FLATTRS_SUCCESS and SAVE_FLATTRS.
No description provided.