Dequeue limbo resolutions when their respective queries are stopped #4395
Conversation
🦋 Changeset detectedLatest commit: 03a9c22 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 |
.changeset/eight-bananas-nail.md
Outdated
| '@firebase/firestore': patch | ||
| --- | ||
|
|
||
| Fix a bug where local cache inconsistencies were unnecessarily being resolved. |
| */ | ||
| enqueuedLimboResolutions: DocumentKey[] = []; | ||
| enqueuedLimboResolutions = new Set<string>(); |
There was a problem hiding this comment.
We should maybe mention that we can use this datastructure in JS because it preserves insertion order.
| const expectedLimboDocs = Array.from(this.expectedEnqueuedLimboDocs, key => | ||
| key.path.canonicalString() | ||
| ).sort(); | ||
| expect(actualLimboDocs).to.deep.equal(expectedLimboDocs); |
There was a problem hiding this comment.
To keep some of the descriptiveness of the original error, you should add a message argument (to the last function call) that mentions that this codepath validates limbo resolutions. Otherwise, an error will show up, but it will not be all that helpful.
Furthermore, you should use to.have.members and remove sort().
| const doc2 = doc('collection2/doc', 1000, { key: 2 }); | ||
| const query2 = query('collection2'); | ||
| const filteredQuery2 = query('collection2', filter('key', '==', 2)); | ||
| const filteredQuery3 = query('collection2', filter('key', '>=', 2)); |
There was a problem hiding this comment.
Maybe:
s/filteredQuery2/filteredQuery2a
s/filteredQuery3/filteredQuery2b
| .userListens(query1) | ||
| .watchAcksFull(query1, 1000, doc1) | ||
| .expectEvents(query1, { added: [doc1] }) | ||
| .userUnlistens(query1) | ||
| .userListens(filteredQuery1) | ||
| .expectEvents(filteredQuery1, { added: [doc1], fromCache: true }) | ||
| .watchAcksFull(filteredQuery1, 1001) | ||
| .expectLimboDocs(doc1.key) |
There was a problem hiding this comment.
If you flip the queries around, the test becomes easier to follow. Here I am now asking: "Doesn't it make sense that the filtered query filters out doc1?" If you flip it and run the filteredQuery before the fullQuery (which might be a better name) then the test becomes a bit more obvious.
Applies everywhere.
There was a problem hiding this comment.
I see what you mean. I had actually originally written it that way. But then I added a third filtered query to some of the tests and it seemed to be a detriment to readability to issue a filtered query followed by a full query followed by another filtered query. For consistency between the test cases, I modified all of the tests to issue the full queries first.
What are your thoughts on the following options to address this:
- Leave it as-is, with the full queries issued first.
- Always issue the filtered queries first, unless there are two filtered queries, in which case issue the full queries first.
- Find another way to put the same document into limbo with two different queries, maybe with a limit query?
There was a problem hiding this comment.
I am going with "one of the above seems ok". I think the argument for consistency makes sense, and the test can generally be followed, as long as the person looking at it also looks at the actual contents of the docs/queries.
| // verify that doc2 *is* removed from the limbo resolution queue. | ||
| .userUnlistens(filteredQuery2) | ||
| .expectLimboDocs(doc1.key) | ||
| .expectEnqueuedLimboDocs() |
There was a problem hiding this comment.
Nit: Both of the tests below seem to be subsets of this test. I would therefore move this test under the other tests to first verify the basic implementation before going into the more completed cases.
|
Let me know if you change the spec tests so much that you want another review. |
I had a few questions based on the comments you left. |
Binary Size ReportAffected SDKs
Test Logs |
Fix a bug where enqueued limbo resolutions are left in the queue even after all targets that care about their resolutions are stopped. This erroneous behavior was found in and reported against the Android SDK (firebase/firebase-android-sdk#2311) but also applies to the Web and iOS SDKs. This fix will be ported to those other SDKs. This bug was introduced when limbo resolution throttling was implemented almost a year ago (#2790).