search

ReallySmallSoftware / cordova-plugin-firestore

A Google Firebase Firestore plugin to enable realtime synchronisation between app and cloud and automatically handle limited connectivity.
Other
22 stars 10 forks source link

DocumentRef.collection should return regular collection to allow recursion #16

Open jggc opened 5 years ago

jggc commented 5 years ago

Expected Behavior

db.collection('rootCollection') // Regular CollectionReference
  .doc('parentDocument') // Regular DocumentReference
  .collection('subCollection') // Regular CollectionReference with /rootCollection/parentDocument/subCollection/ path
  .doc('childDocument') // Regular DocumentReference with /rootCollection/parentDocument/subCollection/subDocument path

Actual Behavior

db.collection('root') // Regular CollectionReference
  .doc('parent') // Regular DocumentReference
  .collection('sub') // Incomplete SubCollection implementation
  .doc('child') // Incomplete SubCollectionDocument implementation

Specifications

My specific need is to be able to do an onSnapshot on subcollections which is not currently possible since DocumentReference.collection returns a SubCollection which ends upd returning DocumentOfSubCollectionReference which does not implement onSnapshot, collection and others.

The problem is architectural : Firestore works with data structure recursion to allow subcollections to have the same functionality as root collections, and this plugin does not follow that pattern.

I tested by passing a subcollection path to a CollectionReference and it works fine so I guess it would be pretty easy to refactor this plugin to follow the same pattern as the official firestore sdks :

I am ready to implement that pattern as I need it to work in my application ASAP to work around the various issues we have currently with firebase-js-sdk in cordova.

So the question is : Why has it not been implemented this way yet? Is there something I am missing?

ReallySmallSoftware commented 5 years ago

The subcollection implementation was done by another collaborator. I will add this to my backlog to investigate but I have a number of other requests so it won't be that soon I.m afraid.

jggc commented 5 years ago

As I said I am ready to implement it myself, in fact I already started.

Thanks a lot for the input but could you just clarify : from what you are seeing there is no reason that you know of that would prevent using recursion? I'm currently pretty much 50% there, adding a basic test setup to the project at the same time.

Another question: Is there a reason to use CamelCase imports in the Javascript when file names are underscored?

ReallySmallSoftware commented 5 years ago

No reason I can see. I started to look at it last night and it looks fairly straightforward I think. Most of the changes are in the js.

On Thu, 11 Apr 2019, 21:36 Jean-Gab, notifications@github.com wrote:

As I said I am ready to implement it myself, in fact I already started.

Thanks a lot for the input but could you just clarify : from what you are seeing there is no reason that you know of that would prevent using recursion? I'm currently pretty much 50% there, adding a basic test setup to the project at the same time.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ReallySmallSoftware/cordova-plugin-firestore/issues/16#issuecomment-482301772, or mute the thread https://github.com/notifications/unsubscribe-auth/AEEdX-qHbcbBGj6340KidIMLqNyuY573ks5vf5zVgaJpZM4cpn-G .

ReallySmallSoftware commented 5 years ago

I have pushed some changes to github. I believe this works but I have only tested on Android, not iOS, so I will not be releasing this until I have tested it more thoroughly.

This supports:

db.get().doc("/my/document/path"); db.get().collection("my").doc("collection").collection("path").doc("document"); db.get().collection("my/collection/path").doc("document");

Or at I believe it does as it seems to work with my test harness application.