Skip to content

Using Compat-Layer for IndexedDB persistence #3901

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Using Compat-Layer for IndexedDB persistence #3901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 19 additions & 19 deletions packages/firestore/exp/src/api/components.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
* limitations under the License.
*/

import { FirebaseFirestore } from './database';
import { FirebaseFirestore, FirestoreCompat } from './database';
import {
MemoryOfflineComponentProvider,
OfflineComponentProvider,
Expand Down Expand Up @@ -44,28 +44,27 @@ const LOG_TAG = 'ComponentProvider';
// Instance maps that ensure that only one component provider exists per
// Firestore instance.
const offlineComponentProviders = new Map<
FirebaseFirestore,
FirestoreCompat,
OfflineComponentProvider
>();
const onlineComponentProviders = new Map<
FirebaseFirestore,
FirestoreCompat,
OnlineComponentProvider
>();

export async function setOfflineComponentProvider(
firestore: FirebaseFirestore,
firestore: FirestoreCompat,
offlineComponentProvider: OfflineComponentProvider
): Promise<void> {
firestore._queue.verifyOperationInProgress();

logDebug(LOG_TAG, 'Initializing OfflineComponentProvider');
const configuration = await firestore._getConfiguration();
await offlineComponentProvider.initialize(configuration);
firestore._setCredentialChangeListener(user =>
// TODO(firestorexp): This should be a retryable IndexedDB operation
firestore._queue.enqueueAndForget(() =>
// TODO(firestorexp): Make sure handleUserChange is a no-op if user
// didn't change
handleUserChange(offlineComponentProvider.localStore, user)
)
firestore._queue.enqueueRetryable(async () => {
await handleUserChange(offlineComponentProvider.localStore, user);
})
);
// When a user calls clearPersistence() in one client, all other clients
// need to be terminated to allow the delete to succeed.
Expand All @@ -77,24 +76,23 @@ export async function setOfflineComponentProvider(
}

export async function setOnlineComponentProvider(
firestore: FirebaseFirestore,
firestore: FirestoreCompat,
onlineComponentProvider: OnlineComponentProvider
): Promise<void> {
firestore._queue.verifyOperationInProgress();

const configuration = await firestore._getConfiguration();
const offlineComponentProvider = await getOfflineComponentProvider(firestore);

logDebug(LOG_TAG, 'Initializing OnlineComponentProvider');
const configuration = await firestore._getConfiguration();
await onlineComponentProvider.initialize(
offlineComponentProvider,
configuration
);
// The CredentialChangeListener of the online component provider takes
// precedence over the offline component provider.
firestore._setCredentialChangeListener(user =>
// TODO(firestoreexp): This should be enqueueRetryable.
firestore._queue.enqueueAndForget(() =>
firestore._queue.enqueueRetryable(() =>
remoteStoreHandleCredentialChange(
onlineComponentProvider.remoteStore,
user
Expand All @@ -105,8 +103,9 @@ export async function setOnlineComponentProvider(
onlineComponentProviders.set(firestore, onlineComponentProvider);
}

async function getOfflineComponentProvider(
firestore: FirebaseFirestore
// TODO(firestore-compat): Remove `export` once compat migration is complete.
export async function getOfflineComponentProvider(
firestore: FirestoreCompat
): Promise<OfflineComponentProvider> {
firestore._queue.verifyOperationInProgress();

Expand All @@ -121,8 +120,9 @@ async function getOfflineComponentProvider(
return offlineComponentProviders.get(firestore)!;
}

async function getOnlineComponentProvider(
firestore: FirebaseFirestore
// TODO(firestore-compat): Remove `export` once compat migration is complete.
export async function getOnlineComponentProvider(
firestore: FirestoreCompat
): Promise<OnlineComponentProvider> {
firestore._queue.verifyOperationInProgress();

Expand Down Expand Up @@ -183,7 +183,7 @@ export async function getLocalStore(
* when the Firestore instance is terminated.
*/
export async function removeComponents(
firestore: FirebaseFirestore
firestore: FirestoreCompat
): Promise<void> {
const onlineComponentProviderPromise = onlineComponentProviders.get(
firestore
Expand Down
34 changes: 26 additions & 8 deletions packages/firestore/exp/src/api/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ import {
setOnlineComponentProvider
} from './components';
import { DEFAULT_HOST, DEFAULT_SSL } from '../../../lite/src/api/components';
import { DatabaseInfo } from '../../../src/core/database_info';
import { DatabaseId, DatabaseInfo } from '../../../src/core/database_info';
import { AutoId } from '../../../src/util/misc';
import { User } from '../../../src/auth/user';
import { CredentialChangeListener } from '../../../src/api/credentials';
Expand All @@ -76,14 +76,29 @@ export interface Settings extends LiteSettings {
cacheSizeBytes?: number;
}

// TODO(firestore-compat): This interface exposes internal APIs that the Compat
// layer implements to interact with the firestore-exp SDL. We can remove this
Copy link
Contributor

Choose a reason for hiding this comment

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

s/SDL/SDK/?

There's a lot of "compat" in this comment but it seems like eventually some of this will be permanent. I wonder if it's worth working toward the future name? Or is "compat" it?

Basically, my concern is that eventually the classic SDK will be fully implemented in terms of the exp SDK and this mechanism of gaining access to internal-only functionality will be permanent. Will this internal-only access always be called "compat"? In iOS and C++ we've called these internal-only extensions "Internal" (e.g. the contents of Firestore/Source/API/Firestore+Internal.h).

What are your thoughts here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The official name for this new API layer is "compat" (I don't like it all that much, but I was told there is precedent across other products). I hope that we can implement the FirestoreCompat layer purely in terms of the tree-shakeable API and that we don't need to keep an internal translation layer. Will see if this pans out.

This specific instance of FirestoreCompat is temporary. The end goal is that all tree-shakeable functions can use the compat APIs and convert to the tree-shakeable types transparently. There is doc here: go/firebase-next-compat

// class once we have an actual compat class for FirebaseFirestore.
export interface FirestoreCompat {
readonly _initialized: boolean;
readonly _terminated: boolean;
readonly _databaseId: DatabaseId;
readonly _persistenceKey: string;
readonly _queue: AsyncQueue;
_getSettings(): Settings;
_getConfiguration(): Promise<ComponentConfiguration>;
_delete(): Promise<void>;
_setCredentialChangeListener(listener: (user: User) => void): void;
}

/**
* The Cloud Firestore service interface.
*
* Do not call this constructor directly. Instead, use {@link getFirestore()}.
*/
export class FirebaseFirestore
extends LiteFirestore
implements _FirebaseService {
implements _FirebaseService, FirestoreCompat {
readonly _queue = new AsyncQueue();
readonly _persistenceKey: string;
readonly _clientId = AutoId.newId();
Expand All @@ -103,7 +118,10 @@ export class FirebaseFirestore
super(app, authProvider);
this._persistenceKey = app.name;
this._credentials.setChangeListener(user => {
this._user = user;
if (!this._user.isEqual(user)) {
this._user = user;
this._credentialListener(user);
}
this._receivedInitialUser.resolve();
});
}
Expand Down Expand Up @@ -250,7 +268,7 @@ export function getFirestore(app: FirebaseApp): FirebaseFirestore {
* @return A promise that represents successfully enabling persistent storage.
*/
export function enableIndexedDbPersistence(
firestore: FirebaseFirestore,
firestore: FirestoreCompat,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth nothing somewhere that these function signatures intentionally differ from the public versions in the .d.ts file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They should go back to the public API when once FirebaseFirestore is fully migrated to a compat-style class.

persistenceSettings?: PersistenceSettings
): Promise<void> {
verifyNotInitialized(firestore);
Expand Down Expand Up @@ -297,7 +315,7 @@ export function enableIndexedDbPersistence(
* storage.
*/
export function enableMultiTabIndexedDbPersistence(
firestore: FirebaseFirestore
firestore: FirestoreCompat
): Promise<void> {
verifyNotInitialized(firestore);

Expand Down Expand Up @@ -326,7 +344,7 @@ export function enableMultiTabIndexedDbPersistence(
* but the client remains usable.
*/
function setPersistenceProviders(
firestore: FirebaseFirestore,
firestore: FirestoreCompat,
onlineComponentProvider: OnlineComponentProvider,
offlineComponentProvider: OfflineComponentProvider
): Promise<void> {
Expand Down Expand Up @@ -415,7 +433,7 @@ export function canFallbackFromIndexedDbError(
* cleared. Otherwise, the promise is rejected with an error.
*/
export function clearIndexedDbPersistence(
firestore: FirebaseFirestore
firestore: FirestoreCompat
): Promise<void> {
if (firestore._initialized && !firestore._terminated) {
throw new FirestoreError(
Expand Down Expand Up @@ -531,7 +549,7 @@ export function terminate(firestore: FirebaseFirestore): Promise<void> {
return firestore._delete();
}

function verifyNotInitialized(firestore: FirebaseFirestore): void {
function verifyNotInitialized(firestore: FirestoreCompat): void {
if (firestore._initialized || firestore._terminated) {
throw new FirestoreError(
Code.FAILED_PRECONDITION,
Expand Down
Loading