-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Real-time Sync: Add base traits/interfaces #508
Conversation
9ba2d75
to
503835e
Compare
We ensure that the prost-generated files match those which have been committed
503835e
to
d658334
Compare
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.
Looks good. Dropped some comments.
async fn get_changes_since(&self, record_index: u64) -> Result<Vec<Record>>; | ||
|
||
/// Adds a record to the remote | ||
async fn set_record(&self, data: SyncData) -> Result<()>; |
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.
Since we are using a list of Records as response to get_changes_since I think we should stick to that structure also on set_record. SyncData seems a higher abstraction.
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.
That's actually been changed in #509, see here. I opted for having get_changes_since
also decode the data.
Also, the reason why set_record
does not take a record itself is because we populate the id
and version
fields automatically:
id
gets the latestRecordId from the local databaseversion
is the current schema verison
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.
@hydra-yse it is hard to review this PR by looking at changes in another PR.
I think either merge both into one PR or move the fixes that are related here to this PR.
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.
Sure, I'll merge the new interface definition here.
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.
The fact that we have a trait with set_record & get_changes_since means we have another implementation that will use these which will be triggered by the app. I think we need to see that implementation before we can determine if we need the interface to look like this.
I am OK with merging both PRs together, move to the implementation of the sync itself (at least partial) so we can review that effectively.
Closes #501