-
Notifications
You must be signed in to change notification settings - Fork 199
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
feat: Datastore use configurable syncMaxRecords and syncPageSize #388
feat: Datastore use configurable syncMaxRecords and syncPageSize #388
Conversation
} | ||
|
||
} | ||
|
||
private func updateModelSyncMetadata() { | ||
private func updateModelSyncMetadata(lastSyncTime: Int?) { |
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.
I'm not seeing that you are using lastSyncTime
in this method?
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.
On line 182. But hmm.. seems like it's not a good idea to overlap the local function variable lastSyncTime
with the instance variable of lastSyncTime
.
We were previously always setting the lastSyncTime
, which is a bug, because we can have a case where we need to paginate, and when we paginate through a base query, we do not want lastSyncTime
to be populated.
But, this has made me think....to make things more clear, it seems like we don't need to keep lastSyncTime
as an instance variable.
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.
Updated to remove instance variable, and will hopefully make things clearer! thanks for the input!
bf03452
to
02a09e7
Compare
1c915b4
to
90b6d5b
Compare
02a09e7
to
ea5f132
Compare
90b6d5b
to
ef108d0
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.
Consider adding a backlog item for the TODO, otherwise LGTM
AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/InitialSync/InitialSyncOperation.swift
Show resolved
Hide resolved
52bd777
to
058da3e
Compare
ef108d0
to
2bec581
Compare
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.