Add isEqual() and port DocumentReference array test. #222
Conversation
mikelehen
requested review from
jshcrowthe,
schmidt-sebastian and
wilhuff
as code owners
| * @param other The `DocumentReference` to compare against. | ||
| * @return true if this `DocumentReference` is equal to the provided one. | ||
| */ | ||
| isEqual(other: DocumentReference): boolean; |
There was a problem hiding this comment.
Maybe this is gas on a fire, but why isEqual? Within our codebase I see usages like these:
equals 187
isEqual 53
Looking more closely, the 53 usages of isEqual are just database's Query.isEqual and its tests (some some checked in logs and Firestore's Query.isEqual).
It seems far more conventional to call this method equals.
(BTW: I'm finding usages like this)
find . \( -name node_modules \
-o -name dist \
-o -name \*.js \
-o -name \*.map \) -prune -o -type f -print0 \
| xargs -0 grep isEqualThere was a problem hiding this comment.
Okay, discussed offline, and it's clear that there's no consensus in the world about isEqual vs equals. The much larger incidence of equals within our code are internal APIs.
Let's file a clean up bug to standardize on isEqual so that customDeepEqual need not check both.
| * @param other The `DocumentReference` to compare against. | ||
| * @return true if this `DocumentReference` is equal to the provided one. | ||
| */ | ||
| isEqual(other: DocumentReference): boolean; |
There was a problem hiding this comment.
Okay, discussed offline, and it's clear that there's no consensus in the world about isEqual vs equals. The much larger incidence of equals within our code are internal APIs.
Let's file a clean up bug to standardize on isEqual so that customDeepEqual need not check both.
|
Josh, can you approve for the .d.ts change in the firebase package? |
This is a port of the tests for the fix I made for Android (and implementing isEqual). Note that the isEqual() API was previously discussed (when it was added for RTDB). See internal issue b/62143881 for more details.