Using Compat-Layer for IndexedDB persistence #3901
Conversation
|
Binary Size ReportAffected SDKs
Test Logs
|
Size Analysis ReportAffected ProductsNo changes between base commit (79b0493) and head commit (ffc0b64). Test Logs
|
schmidt-sebastian
force-pushed
the
mrschmidt/enablepersistence-compat
branch
4 times, most recently
from
a027964 to
7fc4e46
Compare
schmidt-sebastian
changed the title
schmidt-sebastian
force-pushed
the
mrschmidt/enablepersistence-compat
branch
from
cba3e1c to
0767aa6
Compare
| @@ -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 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| @@ -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, | |||
There was a problem hiding this comment.
Is it worth nothing somewhere that these function signatures intentionally differ from the public versions in the .d.ts file?
There was a problem hiding this comment.
They should go back to the public API when once FirebaseFirestore is fully migrated to a compat-style class.
| // throws an exception. | ||
| this.ensureClientConfigured(); | ||
| await this._firestoreClient!.terminate(); | ||
|
|
||
| this._queue.enterRestrictedMode(); |
There was a problem hiding this comment.
This and similar changes pulling FirestoreClient code into the API layer seem like a fairly radical departure from the way the other SDKs work.
Notably, on other platforms we have an API layer that serves as a stable interface to user code and we have most of the functionality in places like FirestoreClient. I would expect that even after we transition to tree-shakeable form, we'd still have much of the same structure, except that instead of a FirestoreClient class with methods, it would be a FirestoreClient struct with free functions. Moving this code out of FirestoreClient into the classic API layer seems like the wrong move--this code should end up somewhere in the exp code in the FirestoreClient equivalent over there. If we don't do that, isn't this going to have to be duplicated there?
There was a problem hiding this comment.
I addressed this comment in a new PR and will abandon this one for now: #3974
This PR pulls the Component Provider from firebase-exp into the legacy SDK. The main challenge is that some of the functionality in FirestoreClient now needs to live in FirebaseFirestore itself (the firestore-exp SDK doesn't use FirestoreClient since it is not tree-shakeable).
This PR passes all tests, but some of the changes here cannot be tested with our current test setup. I verified manually that persistence fallback works and that auth tokens are properly swapped as users sign in and out (by using two users in a web app and enforcing via security rules that each user can only write into their region of Firestore)