-
-
Notifications
You must be signed in to change notification settings - Fork 597
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
LocalDatastore can update from server #734
Conversation
Codecov Report
@@ Coverage Diff @@
## master #734 +/- ##
==========================================
+ Coverage 88.74% 88.83% +0.08%
==========================================
Files 53 53
Lines 4584 4620 +36
Branches 1064 1069 +5
==========================================
+ Hits 4068 4104 +36
Misses 516 516
Continue to review full report at Codecov.
|
Did you check how the other SDKs implémented those features? I’m not sure the server is properly equipped to ‘sync’ data when it’s been off sync? Sent with GitHawk |
If by other SDK's you mean iOS and android LDS, I don't believe they have this kind of feature. They do have
I'm not looking for a true sync feature with this PR. Just when network reconnect perform a query. We could add an optional scheduler that will sync it periodically or with user could call this function whenever they wanted to sync data |
That’s not what I meant, when you will get network connection again, you will attempt to save object, and may override previous operations. But I guess you already knew that. Sent with GitHawk |
The REstController also is not the best spot to get this feature in, as it’s responsibility is solely to connect to the API. It has nothing to do with the LDS. Sent with GitHawk |
That makes sense. When do you believe the best implementation is? I would like to the SDK to handle it. Any advice would be appreciated @acinader Thoughts |
@flovilmart How does this look? |
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.
trying to wrap my head around this....
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.
lgtm. one question about the logger.
this.isSyncing = false; | ||
} catch(error) { | ||
console.log('Error syncing LocalDatastore'); // eslint-disable-line | ||
console.log(error); // eslint-disable-line |
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.
any reason not to use parse's logger so this stuff shows up in aggregated logs?
see middlewares, MogoTransform, PromiseRouter, etc. for examples
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.
something like
const Logger = require('parse-server/lib/logger');
Logger.logger.error(`Error syncing LocalDatastore: ${error}`);
console.log('Error syncing LocalDatastore', error);
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.
Any reason to not follow the exact pattern we have already?
i.e.
import log from './logger';
...
log.error('message', object);
...
and remove the console.log and the eslint exception comment?
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.
Thats the parse-server repo, there isn't a logger in this repo that I know of.
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.
ha. riiiight. yeah, lgtm. ;) . I don't use the client js sdk is the only lame excuse i have for that one.
Allows for updating LocalDatastore with server data
Any advice would be helpful