Compat class for QuerySnapshot #4058
Conversation
|
| @@ -403,17 +402,108 @@ export class QuerySnapshot<T = DocumentData> { | |||
| !this._cachedChanges || | |||
| this._cachedChangesIncludeMetadataChanges !== includeMetadataChanges | |||
| ) { | |||
| this._cachedChanges = changesFromSnapshot<QueryDocumentSnapshot<T>>( | |||
| this._cachedChanges = this.changesFromSnapshot( | |||
There was a problem hiding this comment.
There's a nontrivial amount of code here and if you don't actually use docChanges this could be tree-shaken away. This change to make changesFromSnapshot a member works against ever being able to make docChanges a free function in our public API.
To me this suggests a general design principle of trying to absolutely minimize the number of member functions we keep in classes.
In this case, it seems like it would be straightforward to transplant the changesFromSnapshot function here.
There was a problem hiding this comment.
I updated this, but I am not sure if this is a stylistic win (and probably not in terms of code size(. It does reduce the amount of code changes in this PR, even though I replaced the factory-function signature with an inline instantiation of the now "hardcoded" QueryDocumentSnapshot.
There was a problem hiding this comment.
FWIW I constantly struggle with my visceral dislike of the style we've adopted to make tree-shakeable APIs a reality. It's ugly, I don't like it, but I have to get over it because this is where JavaScript is going.
Part of my reasoning for pushing for a blanket rule on member vs free is to remove this decision from the realm of style. I see the local style suboptimality of free functions as a net win because it makes things more consistent and removes the need to reason about how to add the next function.
| @@ -403,17 +402,108 @@ export class QuerySnapshot<T = DocumentData> { | |||
| !this._cachedChanges || | |||
| this._cachedChangesIncludeMetadataChanges !== includeMetadataChanges | |||
| ) { | |||
| this._cachedChanges = changesFromSnapshot<QueryDocumentSnapshot<T>>( | |||
| this._cachedChanges = this.changesFromSnapshot( | |||
There was a problem hiding this comment.
FWIW I constantly struggle with my visceral dislike of the style we've adopted to make tree-shakeable APIs a reality. It's ugly, I don't like it, but I have to get over it because this is where JavaScript is going.
Part of my reasoning for pushing for a blanket rule on member vs free is to remove this decision from the realm of style. I see the local style suboptimality of free functions as a net win because it makes things more consistent and removes the need to reason about how to add the next function.
No description provided.