Make clearIndexedDbPersistence() work without enableIndexedDbPersistence() #3373
Conversation
🦋 Changeset is good to goLatest commit: 0845047 We got this. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Binary Size ReportAffected SDKs
Test Logs |
schmidt-sebastian
changed the title
schmidt-sebastian
force-pushed
the
mrschmidt/fixclear
branch
from
de138b5 to
ca0261e
Compare
schmidt-sebastian
requested review from
avolkovi,
bojeil-google,
Feiyang1,
hiranya911,
hsubox76,
sam-gc and
scottcrossen
as code owners
schmidt-sebastian
force-pushed
the
mrschmidt/fixclear
branch
from
ca0261e to
ddddda8
Compare
packages/auth/src/args.js
Outdated
| @@ -1,6 +1,6 @@ | |||
| /** | |||
| * @license | |||
| * Copyright 2017 Google Inc. | |||
| * Copyright 2017 Google LLC | |||
There was a problem hiding this comment.
These changes seem unrelated. Revert?
There was a problem hiding this comment.
Removed. I actually tried to remove them earlier, but the pre commit hook brought them back in. I had to disable the precommit hook for this commit.
| @@ -247,7 +254,9 @@ export function clearIndexedDbPersistence( | |||
| firestore: firestore.FirebaseFirestore | |||
| ): Promise<void> { | |||
| const firestoreImpl = cast(firestore, Firestore); | |||
| return firestoreImpl._clearPersistence(); | |||
| return firestoreImpl._clearPersistence((databaseId, persistenceKey) => { | |||
| return clearPersistence(buildStoragePrefix(databaseId, persistenceKey)); | |||
There was a problem hiding this comment.
This code reads as if it's circular unless you pay careful attention. If clearPersistence were named indexedDbClearPersistence it would be clearer that the higher level API was delegating to the lower-level implementation.
I realize this is getting at that type-operation naming discussion we started earlier so feel free to defer if you must, but I think it's really hurting the ability to understand what's going on here while reading the code directly. Sure you can go look at the imports but that breaks flow. Reading code is hard enough and we should strive to make it easy to understand.
There was a problem hiding this comment.
I stumbled across this earlier too, but didn't act upon my confusion. I think a minimum bar for code quality should be that at the very least the person writing it shouldn't be confused...
Renamed to indexedDbClearPersistence and indexedDbStoragePrefix.
In the current firestore-exp client, clearIndexedDbPersistence() cannot be called on an uninitialized instance since the IndexedDbComponentProvider is not registered. This PR changes the plumbing so that clearIndexedDbPersistence() brings all logic that it needs to perform the delete without pulling in IndexedDbPersistence or the IndexedDbComponentProvider.
Note that I did update the dependencies file, but the firestore-exp client is currently so bad at being "tree-shaken" that the dependencies don't make much sense.