refactor sync to use appStore change listener #8480
Conversation
| if (syncEnabled()) { | ||
| state = syncUtil.updateSiteCache(state, action.destinationDetail) | ||
| } | ||
| break | ||
| case appConstants.APP_APPLY_SITE_RECORDS: |
There was a problem hiding this comment.
this was moved from appStore.js to here because it is a site reducer
app/sync.js
Outdated
|
|
||
| // DELETES are handled in appState because the old object is no longer | ||
| // available by the time emitChanges is received | ||
| // const isDelete = diff.op === 'remove' && path.length === 3 |
js/state/siteUtil.js
Outdated
| @@ -219,10 +219,10 @@ const mergeSiteDetails = (oldSiteDetail, newSiteDetail, tag, folderId, order) => | |||
| * @param tag The tag to add for this site | |||
| * See siteTags.js for supported types. No tag means just a history item | |||
| * @param originalSiteDetail If specified, use when searching site list | |||
| * @param {Function=} syncCallback specified if this change should be synced | |||
| * @param skipSync If specified, this item is not eligible for sync | |||
There was a problem hiding this comment.
after reading the PR a bit, it seems to me that skipSync is a flag set to inform reducers of the original cause for addSite (a browser record vs. a downloaded record). is it worth mentioning that here for clarity?
There was a problem hiding this comment.
Yeah, I think actually we should rename it to something clearer like addedBySync since it is just keeping track of which records were downloaded by sync.
|
Tests are working again. Running this branch, rebased on master a8bc66b I get failures with |
Hm, I think this PR actually uncovered a bug in brave/sync. I will open a PR there. |
and only trigger sync when important site fields are modified. fix #8415 may also fix brave/sync#31 test plan: 0. automated sync tests should pass 1. enable sync and go to https://example.com 2. bookmark it. you should see a message in the console indicating a record was sent. 3. close the page and visit it again. you should not see a message in the console. 4. unbookmark it. you should see a message in the console indicating a record was sent.
The previous way somtimes merged a record into record with a different objectId somehow. Using Object.values guarantees that the final number of merged records will be the same as the number of unique objectIds. Test plan: 1. Sync to the 'rummy tarp...' group sent by slack DM. 2. You should see a bookmark folder 'f1' with two bookmarks in it; previously the bookmarks would be at the top level. 3. checkout brave/browser-laptop#8480 and run "npm run test -- --grep='^Syncing bookmarks'". the tests should pass.
|
brave/sync#84 fixed the syncing bookmark tests for me. once that is merged, i can restart the Travis job and hopefully it will pass. |
and only trigger sync when important site fields are modified.
fix #8415
may also fix brave/sync#31
test plan:
0. automated sync tests should pass
git rebase -ito squash commits (if needed).Test Plan: