-
Notifications
You must be signed in to change notification settings - Fork 147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for applying default converter in withConverter() #1394
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1394 +/- ##
==========================================
+ Coverage 98.28% 98.51% +0.22%
==========================================
Files 32 32
Lines 19341 19374 +33
Branches 1308 1273 -35
==========================================
+ Hits 19010 19086 +76
+ Misses 321 284 -37
+ Partials 10 4 -6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one drive-by comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Some nits, some optional nits :)
We also need to update the types file.
dev/src/collection-group.ts
Outdated
* Firestore. | ||
* @param {FirestoreDataConverter | null} converter Converts objects to and | ||
* from Firestore. Passing in `null` applies the default `DocumentData` typed | ||
* converter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe: Passing in
null removes the current converter.
Up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is clearer since users might not have the concept of a default converter.
dev/src/collection-group.ts
Outdated
): CollectionGroup<U> { | ||
return new CollectionGroup<U>( | ||
this.firestore, | ||
this._queryOptions.collectionId, | ||
converter | ||
converter !== null ? converter : defaultConverter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is NodeJS and we don't care as much about code size, you could also use converter ?? defaultConverter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
dev/system-test/firestore.ts
Outdated
@@ -148,6 +148,7 @@ describe('Firestore class', () => { | |||
}); | |||
|
|||
it('getAll() supports generics', async () => { | |||
randomCol.doc('lo')._converter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops.
dev/test/query.ts
Outdated
// Check that the Post class's toString() override does not apply. | ||
expect(posts.docs[0].data().toString()).to.equal('[object Object]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not do an instanceof
check here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point.
dev/test/query.ts
Outdated
@@ -741,8 +742,7 @@ describe('query interface', () => { | |||
|
|||
const posts = await coll.where('title', '==', 'post').get(); | |||
expect(posts.size).to.equal(1); | |||
// Check that the Post class's toString() override does not apply. | |||
expect(posts.docs[0].data().toString()).to.equal('[object Object]'); | |||
expect(posts.docs[0].data() instanceof Post).to.be.false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to.not.be.an.instanceof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
learn2chai
No description provided.