Skip to content
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

Port FirebaseDataConverter interface #856

Closed
jeantil opened this issue Jan 6, 2020 · 2 comments · Fixed by #828
Closed

Port FirebaseDataConverter interface #856

jeantil opened this issue Jan 6, 2020 · 2 comments · Fixed by #828
Assignees
Labels
api: firestore Issues related to the googleapis/nodejs-firestore API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@jeantil
Copy link

jeantil commented Jan 6, 2020

Is your feature request related to a problem? Please describe.
I recently started storing documents which have fields of type Date using the admin sdk (since I run server side). I was unpleasantly surprised to find that reading the documents back would return these fields as a firebase Timestamp but I can understand why it works that way.
Searching for a way to convert these back to more standard types in a generic way, I found documentation on FirebaseDataConverter which is applicable to DocumentReference#withConverter and then wasted a couple hours trying to understand why I couldn't find this method in my code and upgrading various firebase dependencies in the hope of getting the version with the method only to finally understand that the server side implementation didn't have that.

Describe the solution you'd like
I would thus like for the #withConverter method and the corresponding FirestoreDataConverter api to be implemented in the admin/server side SDK.

Additional context
I find it really confusing that the APIs on the admin and client implementations use exactly the same names but with slightly different apis. Google search results rarely if ever provide enough context to know which implementation the code sample is applicable to, same on stackoverflow.
Having firebase testing actually pull in the client sdk and not having a server side testing lib further increased my confusion by installing both dependencies in node_modules.

Since both codebase describe the same thing, I would like for both apis to be the same with a single documentation. Where some operations are specific to cient or server, they can be tagged as such in the documentation (ideally explaining why they only make sense on one or the other to help users understand the model better.)

@schmidt-sebastian
Copy link
Contributor

schmidt-sebastian commented Jan 6, 2020

@jeantil Thanks for filing this report. I understand your frustration - we try to align these APIs as best as we can, but under the hood the SDKs are very different. The Web API is much richer in some aspects (since the SDK supports persistence and local edits), while in other aspects the Server API supports more functionality (e.g. listDocuments()). While we do our best to hide these differences, in some areas this does not work very well - and @firebase/testing highlights these areas pretty well.

Regarding some of the specifics you mentioned:

  • withConverter was just recently release on Web and is coming to the Server SDK (see feat: add support for Typescript Custom Mapping #828 for the in-progress work).
  • We no longer support returning Date objects from the Server SDK. This is a long coming change that will be coming soon to the Web SDK. We have been encouraging users to pass settings({timestampsInSnapshots: true}) in the Web SDK, which aligns these two behaviors (in fact, in the current version the behavior should only diverge if you explicitly turn off timestampsInSnapshots).

@schmidt-sebastian schmidt-sebastian changed the title Align admin and client API for DocumentReference (and probably the rest) Port FirebaseDataConverter interface Jan 6, 2020
@jeantil
Copy link
Author

jeantil commented Jan 6, 2020

Thank you very much for our answer and your understanding.

I'm glad to know there is work in progress on implementing withConverter. I have subscribed to #828 and will close this issue when it is released.

under the hood the SDKs are very different.

And that's perfectly fine and acceptable
My frustration comes from different and related things having the exact same name :) making any search very difficult to desambiguate. It would be easier for me if all the server SDK types were suffixed or prefixed by Server (ServerFirestore, ServerCollection, ServerDocumentReference) as it would make finding the relevant documentation much easier. Maybe I'm using it wrong and I should really create a normal client with a custom user with full permissions to all documents and collections but it felt kinda like reinventing service accounts so ...

We no longer support returning Date objects from the Server SDK

That's fine, I'm a fairly recent user of firebase and I don't expect much.
On a more conceptual level, not having a symetrical ser/des for a persistence library can kind of break the principle of least surprise... All the more so if there is no built-in and advertised mechanism of restoring the symmetry.

I know I would much prefer that if trying toserialize a date object simply failed or produced a nonsensical result in the store. Then I would know immediately that I have to convert before storing and upon reading values from the store.

Thanks again.

@bcoe bcoe added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Jan 6, 2020
@google-cloud-label-sync google-cloud-label-sync bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/nodejs-firestore API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants