Skip to content

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

Merged
merged 2 commits into from
Jun 24, 2020
Merged

Make LocalStore an interface. #3269

merged 2 commits into from
Jun 24, 2020

Conversation

wu-hui
Copy link
Contributor

@wu-hui wu-hui commented Jun 24, 2020

Make LocalStore an interface and have LocalStoreImpl implements LocalStore. Public fields in LocalStoreImpl then becomes file private, because LocalStoreImpl is not exported.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jun 24, 2020

Binary Size Report

Affected SDKs

  • @firebase/firestore

    Type Base (5de3ec6) Head (e37090c) Diff
    browser 248 kB 248 kB +44 B (+0.0%)
    esm2017 194 kB 194 kB +44 B (+0.0%)
    main 492 kB 493 kB +404 B (+0.1%)
    module 246 kB 246 kB +44 B (+0.0%)
  • @firebase/firestore/memory

    Type Base (5de3ec6) Head (e37090c) Diff
    browser 186 kB 186 kB +22 B (+0.0%)
    esm2017 145 kB 145 kB +22 B (+0.0%)
    main 362 kB 363 kB +214 B (+0.1%)
    module 184 kB 184 kB +22 B (+0.0%)
  • firebase

    Type Base (5de3ec6) Head (e37090c) Diff
    firebase-firestore.js 287 kB 287 kB +44 B (+0.0%)
    firebase-firestore.memory.js 226 kB 226 kB +22 B (+0.0%)
    firebase.js 820 kB 820 kB +44 B (+0.0%)

Test Logs

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a 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
Copy link
Contributor

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)

Copy link
Contributor Author

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.
Copy link
Contributor

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.

Copy link
Contributor Author

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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@wu-hui wu-hui merged commit 9cf49f7 into master Jun 24, 2020
@firebase firebase locked and limited conversation to collaborators Jul 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants