Compat class for Query and CollectionReference #4065
Conversation
|
Changeset File Check ✅
|
| directionStr?: PublicOrderByDirection | ||
| ): Query<T> { | ||
| try { | ||
| // The "as string" cast is a little bit of a hack. `orderBy` accepts the |
There was a problem hiding this comment.
I'm surprised that we can't just make this different in our public typings vs are internal actual types. It seems like we could declare this in the .ts file and just omit it from the .d.ts?
There was a problem hiding this comment.
We are moving away from manually crafted typings. I still need to iron out some final details in #3790, but once that is merged, we will generate typings from source. All other firebase-exp SDKs are doing the same.
| doc(documentPath?: string): DocumentReference<T> { | ||
| try { | ||
| if (documentPath !== undefined) { | ||
| return new DocumentReference( |
There was a problem hiding this comment.
Why doesn't this case work if documentPath is undefined? I thought doc can't tell if you called it as doc(this._delegate, undefined) vs doc(this._delegate).
There was a problem hiding this comment.
I believe we owe this to the genius of @mikelehen.
I added this comment:
// Call `doc` without `documentPath` if `documentPath` is `undefined`
// as `doc` validates the number of arguments to prevent users from
// accidentally passing `undefined`.
| `use 'x' as your first orderBy(), but your first orderBy() is on ` + | ||
| `field 'y' instead.`; | ||
| `use 'x' as your first argument to Query.orderBy(), but your first ` + | ||
| `orderBy() is on field 'y' instead.`; |
There was a problem hiding this comment.
It's somewhat surprising that we replace the first orderBy but not the second. In other cases where they're not spelled the same this seems like it would allow the implementation to leak out.
There was a problem hiding this comment.
JavaScript reg-exp only replace one instance by default. I could use /g here but then the message will be harder to parse (or has to be rewritten even more).
There was a problem hiding this comment.
Feedback addressed (unfortunately as part of the merge commit: b5f4490, which merges the WriteBatch removal)
| doc(documentPath?: string): DocumentReference<T> { | ||
| try { | ||
| if (documentPath !== undefined) { | ||
| return new DocumentReference( |
There was a problem hiding this comment.
I believe we owe this to the genius of @mikelehen.
I added this comment:
// Call `doc` without `documentPath` if `documentPath` is `undefined`
// as `doc` validates the number of arguments to prevent users from
// accidentally passing `undefined`.
| directionStr?: PublicOrderByDirection | ||
| ): Query<T> { | ||
| try { | ||
| // The "as string" cast is a little bit of a hack. `orderBy` accepts the |
There was a problem hiding this comment.
We are moving away from manually crafted typings. I still need to iron out some final details in #3790, but once that is merged, we will generate typings from source. All other firebase-exp SDKs are doing the same.
| `use 'x' as your first orderBy(), but your first orderBy() is on ` + | ||
| `field 'y' instead.`; | ||
| `use 'x' as your first argument to Query.orderBy(), but your first ` + | ||
| `orderBy() is on field 'y' instead.`; |
There was a problem hiding this comment.
JavaScript reg-exp only replace one instance by default. I could use /g here but then the message will be harder to parse (or has to be rewritten even more).
|
LGTM |
This series of PRs is coming to an end.
This PR adds Compat classes for Query and CollectionReference and is split across two commits. The first one creates the Compat classes, the second one moves code from src/api/database.ts to lite/src/api/reference.ts as the helper functions are now no longer needed by the classic API.