samuelgozi / firebase-firestore-lite

A lightweight cloud firestore library for the browser
MIT License
212 stars 12 forks source link

fix: mask type definition in Reference #50

Closed KoichiKiyokawa closed 3 years ago

KoichiKiyokawa commented 3 years ago

Thank you for such a great library :tada:

In firebase doc, mask query parameter should be DocumentMask(you can see at this page)

And, the type of DocumentMask is { fieldPaths: string[] } (this page)

So, mask type should be { fieldPaths: string[] }, but now you define string[].

I used the following code to confirm that it works.

// In posts collection, there is a document `documentID: post1, data: { a: 1, b: 2, secret: 3 }`
db.ref('posts/post1').get({ mask: { fieldPaths: ['a', 'b'] } }) // OK
db.ref('posts/post1').get({ mask: ['a', 'b'] }) // Error: Invalid JSON payload received. Unknown name "mask": Cannot bind query parameter. 'mask' is a message type.

I'm not very good at writing test code, so sorry for the lack of jest tests in this pull request.

samuelgozi commented 3 years ago

Hi and thank you for your contribution! It seems alright to me, but one question before I merge. Do you think the proposed API is good, or would you prefer it to work with the old API?

I think that the current API (that doesn't work) feels cleaner. So I think that maybe we should still accept current arguments, but convert them behind the scenes to the one required by the firebase REST API in order for it to work.

So, what do you think?

KoichiKiyokawa commented 3 years ago

Thank you for your comment!

I hadn't thought of the idea of changing the internal processing instead of the type definition 👍 I also think that the current API is easier to use.

If the option name is mask, it will confuse people who are familiar with firebase rest api(knowing mask.fieldPaths), so it might be a good idea to change the name.

// before
db.ref('posts/post1').get({ mask: ['a', 'b'] })

// after
db.ref('posts/post1').get({ fields: ['a', 'b'] })
                            ^^^^^

You can close this PR.

samuelgozi commented 3 years ago

I fixed the issue but didn't change the name to "fields". I didn't change it because I want all names to be as close as the ones in the REST API. I think it's just easier to maintain that way, and I think the names in the API make sense too. The changes should be un during the next hour in a new minor version.

I added a test for this, but if I missed something please let me know. And thank you for the help!