Resolve syncRecords #26

ayumi · 2017-01-01T03:26:48Z

No description provided.

diracdeltas · 2017-01-01T23:05:22Z

client/recordUtil.js

+   * Browser clients periodically request records, then finds matching browser
+   * objects and IPC calls this function to get resolved browser writes.
+   * @param {SyncRecord} record
+   * @param {Object=} existingObject (e.g. { location: "https://...", title: "cool", ... })


i would prefer the input to be the object that the browser has stored, which may have fields that are not in the SyncRecord. ex: a bookmark has a favicon that is not synced. this function should merge the SyncRecord against that object, leaving the non-synced fields untouched, and return the new object.

ex: browser sends

{ location: foo favicon: bar objectId: baz }

sync record is of the form:

{ location: abc objectId: baz }

the returned merged object is:

{ location: abc objectId: baz favicon: bar }

diracdeltas · 2017-01-01T23:05:22Z

client/recordUtil.js

+   * objects and IPC calls this function to get resolved browser writes.
+   * @param {SyncRecord} record
+   * @param {Object=} existingObject (e.g. { location: "https://...", title: "cool", ... })
+   * @returns {SyncRecord} resolved record which the browser should do


along the lines above: i think this should return an object of the same form as existingObject (plus extra fields from SyncRecord if any), not a SyncRecord.

I think that makes sense when the browser provides an existing object.
Without an existingObject, for example a Create record, Sync would still need to send the SyncRecord to the browser for the browser to create its object.
Do we want resolve() to return multiple "types": browser object w/ merged SyncRecord, original SyncRecord or null?

diracdeltas · 2017-01-01T23:05:22Z

client/recordUtil.js

+        if (existingObject) {
+          return nullIgnore()
+        } else {
+          return record


i feel like this should return the merge of record and existingObject

diracdeltas · 2017-01-01T23:05:22Z

client/recordUtil.js

+        }
+      case proto.actions.UPDATE:
+        if (existingObject) {
+          return record


same as above

diracdeltas · 2017-01-01T23:05:22Z

client/recordUtil.js

+      lastAccessedTime: site.lastAccessedTime || Date.now(),
+      creationTime: site.creationTime || Date.now()
+    }
+    return record.$type.create(Object.assign(


i think this should return just a regular JS object instead of a proto message since the browser doesn't currently do any proto parsing

to confirm: a protobuf object is created here but not encoded into bytes, so if it's sent to the browser via IPC will it be able to access the object?

diracdeltas · 2017-01-01T23:05:22Z

client/recordUtil.js

+    if (!record || record.action !== proto.actions.UPDATE) {
+      throw new Error('Missing UPDATE syncRecord.')
+    }
+    switch (record.objectData) {


seems ok for these all to just return record.objectData + record.objectId


        recordUtil for syncRecord resolution

Try to resolve syncRecord Update to Create when needed

ayumi · 2017-01-05T06:09:39Z

@diracdeltas i've made changes to do resolution of syncRecords + existingObjects. could you take another look?

diracdeltas · 2017-01-05T19:09:58Z

client/constants/messages.js

-   * browser must update its local values with the new sync records, performing
-   * conflict-resolution as necessary.
+   * browser should find objects with matching objectIds then call
+   * RESOLVE_SYNC_RECORDS to perform conflict resolution.
   */
  RECEIVE_SYNC_RECORDS: _, /* @param {string} categoryName, @param {Array.<Object>} records */


i wonder if categoryName is needed any more; browser can infer category from the record fields?

also this should be renamed to something like GET_EXISTING_RECORDS

sync's requestUtil still needs to make separate s3 ListObject requests per category.
so if the user syncs a subset of the available categories it would be faster and we'll also avoid sending browser clients unwanted data.

i see, that means RESOLVE_SYNC_RECORDS and RESOLVED_SYNC_RECORDS will only ever send records in the same category, right? so it seems those should both send a categoryName parameter

correct, those two calls would only call with same-category records.
browser clients can examine each record's objectData to determine what to do with them.

up to you but i think it is slightly neater to send categoryName explicitly because all records will be in the same category

diracdeltas · 2017-01-05T19:14:07Z

package.json

@@ -8,7 +8,7 @@
    "flow": "flow; test $? -eq 0 -o $? -eq 2",
    "build": "yarn upgrade && browserify client/sync.js | uglifyjs - > bundles/bundle.js",
    "browsertest": "yarn start-test; browserify test/client/*.js test/*.js | uglifyjs - | tape-run; yarn stop-test",
-    "browsertest-client": "yarn start-test; browserify test/client/*.js | uglifyjs - | tape-run --browser chrome; yarn stop-test",


was this change intentional?

yes; browsertest-client is helpful for developing client-only code because it runs faster. it's only used in dev, so it's ok to skip uglify as long as you check browsertest before committing.

diracdeltas · 2017-01-05T19:15:05Z

client/constants/messages.js

+   * webview -> browser
+   * browser must update its local values with the resolved sync records.
+   */
+  RESOLVED_SYNC_RECORDS: _, /* @param {Array.<Object>} records */


given that this doesn't have a categoryName parameter, it seems the browser has to infer category from the record object anyway, so categoryName probably isn't needed for RECEIVE_SYNC_RECORDS either


        Add IPC to resolve syncRecords

diracdeltas requested changes Jan 1, 2017

View changes

ayumi force-pushed the feature/resolution branch 4 times, most recently from e926865 to 1ffac21 Jan 4, 2017

recordUtil for syncRecord resolution

258eb04

Try to resolve syncRecord Update to Create when needed

ayumi changed the title ~~WIP syncRecord resolution~~ syncRecord resolution Jan 5, 2017

ayumi changed the title ~~syncRecord resolution~~ resolve syncRecords Jan 5, 2017

ayumi force-pushed the feature/resolution branch from 1ffac21 to be7ae31 Jan 5, 2017

ayumi changed the title ~~resolve syncRecords~~ Resolve syncRecords Jan 5, 2017

diracdeltas reviewed Jan 5, 2017

View changes

diracdeltas approved these changes Jan 5, 2017

View changes

Add IPC to resolve syncRecords

Loading status checks…

016bef4

ayumi force-pushed the feature/resolution branch from be7ae31 to 016bef4 Jan 5, 2017

ayumi merged commit 9b9e901 into staging Jan 5, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

continuous-integration/travis-ci/push The Travis CI build passed
Details

ayumi deleted the feature/resolution branch Jan 5, 2017

brave / sync

Resolve syncRecords #26

Resolve syncRecords #26

ayumi commented Jan 1, 2017 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ayumi commented Jan 5, 2017 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

brave / sync

Join GitHub today

Resolve syncRecords #26

Resolve syncRecords #26

Conversation

ayumi commented Jan 1, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ayumi Jan 3, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ayumi commented Jan 5, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ayumi Jan 5, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ayumi commented Jan 1, 2017 •

edited

ayumi Jan 3, 2017 •

edited

ayumi commented Jan 5, 2017 •

edited

ayumi Jan 5, 2017 •

edited