This repository was archived by the owner on May 21, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 5
Issue #14 - Replace db transaction used when saving flattrs with a redux saga #15
Open
erikvold
wants to merge
1
commit into
flattr:master
Choose a base branch
from
erikvold:redux-save-flattrs
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
File renamed without changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| "use strict"; | ||
|
|
||
| const {Date} = require("global/window"); | ||
|
|
||
| const {db} = require("./index.js"); | ||
|
|
||
| function* save({entity, tabId, title, type, url}) | ||
| { | ||
| let {flattrs} = db; | ||
|
|
||
| if (url) | ||
| { | ||
| flattrs = flattrs.where("url").equals(url); | ||
| } | ||
| else | ||
| { | ||
| flattrs = flattrs.where("entity").equals(entity) | ||
| .and((entry) => entry.url == url); | ||
| } | ||
|
|
||
| let entry = yield flattrs.first(); | ||
| if (!entry) | ||
| { | ||
| entry = { | ||
| entity, title, url, | ||
| timestamps: [Date.now()] | ||
| }; | ||
|
|
||
| yield db.flattrs.add(entry); | ||
| } | ||
| else | ||
| { | ||
| entry.timestamps.push(Date.now()); | ||
|
|
||
| yield db.flattrs.modify(entry); | ||
| } | ||
|
|
||
| return entry; | ||
| } | ||
| exports.save = save; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| "use strict"; | ||
|
|
||
| const { | ||
| SAVE_FLATTRS, | ||
| SAVE_FLATTRS_MERGE_PENDING, | ||
| SAVE_FLATTRS_SUCCESS, | ||
| SUBMIT_FLATTRS, | ||
| SUBMIT_FLATTRS_MERGE_PENDING, | ||
| SUBMIT_FLATTRS_SUCCESS, | ||
|
|
@@ -10,36 +13,67 @@ const { | |
| const {filterFlattrsForURLs} = require("../filters/flattrs"); | ||
|
|
||
| const getFlattrsInitialState = () => ({ | ||
| submitting: [], pending: [] | ||
| save: {saving: [], pending: []}, | ||
| submit: {submitting: [], pending: []} | ||
| }); | ||
|
|
||
| let validFlattrs = (action) => ( | ||
| Array.isArray(action.flattrs) && action.flattrs.length > 0); | ||
|
|
||
| const flattrsReducer = (state = getFlattrsInitialState(), action) => | ||
| { | ||
| let {submitting, pending} = state; | ||
| let diff = {}; | ||
| let newState = {}; | ||
| newState.save = Object.assign({}, state.save); | ||
| newState.submit = Object.assign({}, state.submit); | ||
|
|
||
| switch (action.type) | ||
| { | ||
| case SUBMIT_FLATTRS_MERGE_PENDING: | ||
| diff = {pending: [], submitting: submitting.concat(pending)}; | ||
| case SAVE_FLATTRS: | ||
| if (!validFlattrs(action)) | ||
| { | ||
| break; | ||
| } | ||
| newState.save.pending = state.save.pending.concat(action.flattrs); | ||
| break; | ||
| case SAVE_FLATTRS_MERGE_PENDING: | ||
| newState.save.pending = []; | ||
| newState.save.saving = state.save.saving.concat(state.save.pending); | ||
| break; | ||
| case SAVE_FLATTRS_SUCCESS: | ||
| newState.save.saving = []; | ||
| break; | ||
| case SUBMIT_FLATTRS: | ||
| if (!Array.isArray(action.flattrs) || action.flattrs.length < 1) | ||
| if (!validFlattrs(action)) | ||
| { | ||
| break; | ||
| } | ||
| diff.pending = pending.concat(filterFlattrsForURLs(action.flattrs)); | ||
| newState.submit.pending = | ||
| state.submit.pending.concat(filterFlattrsForURLs(action.flattrs)); | ||
| break; | ||
| case SUBMIT_FLATTRS_MERGE_PENDING: | ||
| newState.submit.pending = []; | ||
| newState.submit.submitting = | ||
| state.submit.submitting.concat(state.submit.pending); | ||
| break; | ||
| case SUBMIT_FLATTRS_FAILURE: | ||
| // we gave up trying to send flattrs to server, so stick the failed | ||
| // flattrs back in to the pending queue for the next event | ||
| diff.pending = pending.concat(submitting); | ||
| newState.submit.pending = | ||
| state.submit.pending.concat(state.submit.submitting); | ||
| // fall through | ||
| case SUBMIT_FLATTRS_SUCCESS: | ||
| diff.submitting = []; | ||
| newState.submit.submitting = []; | ||
| break; | ||
| } | ||
|
|
||
| return Object.assign({}, state, diff); | ||
| return newState; | ||
| }; | ||
| exports.flattrs = flattrsReducer; | ||
|
|
||
| const getFlattrs = (state) => ((state || {}).flattrs || {}); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: I'd suggest writing this as |
||
|
|
||
| const getFlattrsToSave = (state) => (getFlattrs(state).save || {}); | ||
| exports.getFlattrsToSave = getFlattrsToSave; | ||
|
|
||
| const getFlattrsToSubmit = (state) => (getFlattrs(state).submit || {}); | ||
| exports.getFlattrsToSubmit = getFlattrsToSubmit; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| "use strict"; | ||
|
|
||
| const {buffers} = require("redux-saga"); | ||
| const { | ||
| actionChannel, call, put, select, take | ||
| } = require("redux-saga/effects"); | ||
|
|
||
| const { | ||
| SAVE_FLATTRS, | ||
| SAVE_FLATTRS_MERGE_PENDING, | ||
| SAVE_FLATTRS_SUCCESS, | ||
| SUBMIT_FLATTRS | ||
| } = require("../types/flattrs"); | ||
| const {getFlattrsToSave} = require("../reducers/flattrs"); | ||
| const {save} = require("../../database/flattrs/save"); | ||
| const {emit} = require("../../../common/events"); | ||
|
|
||
| function* saveFlattrs() | ||
| { | ||
| yield put({type: SAVE_FLATTRS_MERGE_PENDING}); | ||
|
|
||
| let {saving} = yield select(getFlattrsToSave); | ||
|
|
||
| for (let {entity, tabId, title, type, url} of saving) | ||
| { | ||
| let entry = yield call(save, {entity, tabId, title, type, url}); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if |
||
|
|
||
| yield call(emit, "flattr-added", {flattr: entry, tabId, type}); | ||
|
|
||
| // we don't want to wait for submission to complete here | ||
| yield call(put, {type: SUBMIT_FLATTRS, flattrs: [entry]}); | ||
| } | ||
|
|
||
| yield put({type: SAVE_FLATTRS_SUCCESS}); | ||
| } | ||
|
|
||
| function* watchForSaveFlattrs() | ||
| { | ||
| const buffer = yield call(buffers.dropping, 1); | ||
|
|
||
| const saveFlattrsChan = yield actionChannel(SAVE_FLATTRS, buffer); | ||
|
|
||
| while (true) | ||
| { | ||
| yield take(saveFlattrsChan); | ||
|
|
||
| const {pending, saving} = yield select(getFlattrsToSave); | ||
|
|
||
| // it's possible that a previous handler already submit the flattrs | ||
| if (pending.length < 1 && saving.length < 1) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| yield call(saveFlattrs, {}); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Detail: |
||
| } | ||
| } | ||
| exports.watchForSaveFlattrs = watchForSaveFlattrs; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).