Invoke RemoteStore's credential change listener even when user didn't change #4189
Conversation
schmidt-sebastian
requested review from
Feiyang1,
hiranya911 and
hsubox76
as code owners
🦋 Changeset detectedLatest commit: 0c73ef1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
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 |
| if (!currentUser.isEqual(user)) { | ||
| currentUser = user; | ||
| client.asyncQueue.enqueueRetryable(async () => { | ||
| await handleUserChange(offlineComponentProvider.localStore, user); |
There was a problem hiding this comment.
I moved the user change guard for LocalStore's handleUserChange here. It has always been part of the SyncEngine's call into LocalStore (https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/core/sync_engine.ts#L1084). This part of the code handles the user change when SyncEngine is not yet loaded (for a fully offline client).
I do however think that the guard here doesn't make a huge difference and mostly added it to be technically correct. This line of code is only executed when memory persistence is used without network access. The only sensible use case for this is an offline Firestore experience that is backed by Bundles. Bundles don't contain any mutations, so the user change would always be a no-op here. Once the user adds the first mutation, SyncEngine gets loaded and overwrites this listener.
I probably didn't describe this very well. But at least I tried, right? :)
There was a problem hiding this comment.
It's not unreasonable to have here. The change is queued vs being immediately applied and also provides a tiny bit more predictability to the system.
There was a problem hiding this comment.
@schmidt-sebastian and I discussed the nature of the change offline. LGTM.
| if (!currentUser.isEqual(user)) { | ||
| currentUser = user; | ||
| client.asyncQueue.enqueueRetryable(async () => { | ||
| await handleUserChange(offlineComponentProvider.localStore, user); |
There was a problem hiding this comment.
It's not unreasonable to have here. The change is queued vs being immediately applied and also provides a tiny bit more predictability to the system.
This fixes an issue in the 8.0.2+ Firestore SDK, which only propagates user changes but not credential changes. This means that our WebChannel connection did not get reinitialized if a user ID didn't change, but the user's permission did.
If we get a user change notification, the correct behavior is:
Verified using Sam's repro. Unfortunately, we don't have any testing infrastructure that automatically exercises this code.
Fixes #4175