Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upNo merge on resolve #353
No merge on resolve #353
Conversation
|
To be clear, partial updates were never intended to be supported and unnecessarily complicated the implementation |
| if (record.action === proto.actions.DELETE || | ||
| !object) { | ||
| return record | ||
| } else if (object.action === proto.actions.DELETE) { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
AlexeyBarabash
Oct 29, 2019
Author
Contributor
I think, DELETE is a special case of the operation. When it happens, the object is destroyed and no more sense to perform any next operation on the object.
I chose DELETE wins way after considering this:
- deviceA and deviceB both has synced bookmark
bookmarkA - at almost the same time
bookmarkAis renamed on deviceA and removed on deviceB - what would I expect?
I would expect bookmarkA will be removed on both devices, so I put this into the code.
And what do you think?
This comment has been minimized.
This comment has been minimized.
darkdh
Oct 30, 2019
Member
we still need to consider timestamp because Android and brave-core now save object id even if the bookmark is deleted, which is the measure for preventing duplicate bookmarks when leaving and rejoining the sync chain. So DELETE still need to compete with CREATE or even UPDATE which is your case .
|
Closing as moving to sync v2 |
AlexeyBarabash commentedOct 28, 2019
Fixes #261
With this PR resolve operation becomes much simpler. No merge is performed, always wins the latest record. From the other side, now the lib could not accept partial records, for example UPDATE record for bookmark with the only
titlefield change, all fields should be specified even ones which remain the same.