-
Notifications
You must be signed in to change notification settings - Fork 948
Make LocalStore an interface. #3269
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
Conversation
Binary Size ReportAffected SDKs
Test Logs
|
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.
Very nice! Thank you.
* Implements `LocalStore` interface. | ||
* | ||
* Note: some field defined in this class might have public access level, but | ||
* the class is not exported so they are only accessible from this file. This is |
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.
s/file/module (the JS specific term)
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.
Done.
rejectBatch(batchId: BatchId): Promise<MaybeDocumentMap>; | ||
|
||
/** | ||
* Returns the largest (latest) batch id in mutation queue that is pending server response. |
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.
This line wrap looks off.
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.
Fixed.
* useful to implement optional features (like bundles) in free functions, such | ||
* that they are tree-shakeable. | ||
*/ | ||
class LocalStoreImpl implements LocalStore { |
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.
Do you mind removing the JSDocs that you copied over? It is too easy for them to go out of sync now that they are in two places.
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.
Done.
/** | ||
* An implementation of LocalStore that provides additional functionality | ||
* for MultiTabSyncEngine. | ||
* | ||
* Note: some field defined in this class might have public access level, but | ||
* the class is not exported so they are only accessible from this file. This is |
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.
s/file/module
Please also remove duplicate comments for MultiTabLocalStoreImpl
.
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.
Done.
Make LocalStore an interface and have
LocalStoreImpl implements LocalStore
. Public fields inLocalStoreImpl
then becomes file private, becauseLocalStoreImpl
is not exported.