IjzerenHein / firestorter

Use Google Firestore in React with zero effort, using MobX 🤘
http://firestorter.com
MIT License
378 stars 50 forks source link

Feature: shortcut to get subcollection for a document #89

Open gsouf opened 4 years ago

gsouf commented 4 years ago

The official firestore client gives us ability to get reference to a subcollection in this way:

var messageRef = db.collection('rooms').doc('roomA')
                .collection('messages').doc('message1');

I couldn't find a equivalent way to do that with firestorter. Is it by design or is it something we can add? In that case I'm happy to send a PR

IjzerenHein commented 4 years ago

Hi. You can use use paths like this:

new Collection('rooms/roomA/messages');

And for document as well:

new Document('rooms/roomA/messages/message1');
gsouf commented 4 years ago

Hi @IjzerenHein

yes, that's what I did in the interim, but when you have an existing reference to a document it saves the burden of doing a string concatenation.

new Collection(`${myDocRef.path}/something`);

// vs:

myDocRef.collection('something');
IjzerenHein commented 4 years ago

Right okay, I don't really see much benefit of that myself tbh. But okay, what kind of API update do you have in mind? Please be specific and provide an API proposal.

gsouf commented 4 years ago

@IjzerenHein thinking of adding a method collection that would return a ref to the collection with the path from the parent + collection name, and a method doc returning a ref to the sub document with the parent. Actually the same way as the official firestore client.

IjzerenHein commented 4 years ago

Can you provide some sample code that shows the API usage and how you would use it?

gsouf commented 4 years ago

Sure...

Given we have a doc ref

const docRef = new Document('myCollection/abcd');

This is how we do now:

const subCollectionRef = new Collection(`${docRef.path}/subCollection`);

This is the proposal:

const subCollectionRef =docRef.collection('subCollection`);

Additionnally we could do the following:

// get a document for the subcollection
const subCollectionDocRef = docRef.collection('subCollection').doc('some id');

// ability to nest even deeper
const subSubCollectionDocRef = docRef.collection('subCollection').doc('some id')
   .collection('subSubCollection').doc('som id');

That's equivalent to this sample from official client https://github.com/firebase/snippets-web/blob/c34434c12d6db375f0fb1ed71031126851f199b7/firestore/test.firestore.js#L177-L178

IjzerenHein commented 4 years ago

I've been so free to re-format the code and remove the word "ref" from it. I find "ref" confusing and unnecessary here and implies that it is the same thing as a Firestore Collection/Document reference, which it is not.

Sure...

Given we have a doc

const doc = new Document('myCollection/abcd');

This is how we do now:

const subCollection = new Collection(`${doc.path}/subCollection`);

This is the proposal:

const subCollection = doc.collection('subCollection`);

Additionally we could do the following:

// get a document for the subcollection
const subCollectionDoc = doc.collection('subCollection').doc('some id');

// ability to nest even deeper
const subSubCollectionDoc = doc.collection('subCollection').doc('some id')
   .collection('subSubCollection').doc('som id');

That's equivalent to this sample from official client https://github.com/firebase/snippets-web/blob/c34434c12d6db375f0fb1ed71031126851f199b7/firestore/test.firestore.js#L177-L178

IjzerenHein commented 4 years ago

This is how we do now:

const subCollection = new Collection(`${doc.path}/subCollection`);

There is actually a slightly better way of doing this, that also deals with dynamically changing paths, which will be relevant in the next comment. You can also do this:

const subCollection = new Collection(() => `${doc.path}/subCollection`);

This will cause the collection to automatically update its path whenever the path in the document changes. This can be very useful, especially when you have a chain of collections/document that depend on each other. Changing the one at the top can cause child collections/documents to automatically update.

IjzerenHein commented 4 years ago

Generally, I like your idea. I think the syntax is neat and compact. I do see some issues/questions though that we need to think about:

  1. As I mentioned before, you can use "reactive" paths as the source for a collection or document. When doing doc.collection('bla'), we could create it in two ways:
class Document {
  collection(path: string) {
    return new Collection(`${this.path}/${path}`);
  }
}

or like this:

class Document {
  collection(path: string) {
    return new Collection(() => `${this.path}/${path}`);
  }
}

Or possibly allow both through an extra argument. Anyway, we would have to choose a default one.

  1. Secondly. We'd need to support Generics for this as well. So imagine that your document or collection, uses a certain type:
interface MessageData {
  userId: string,
  test: string
};

const messages = chat.collection<DocData>('messages');

When you omit the generic-type, we could use any.

What are your thoughts?

gsouf commented 4 years ago

Thanks for the explanations.

Actually I'm not sure what default would fit better between the simple string approach vs the reactive path. I just started to work on an application using firestorter so I still lack of experience with it to give the right answer. If you recommend usage of reactive path then we probably want to use this one. Because the library is focused on doing things automatically then it's probably the right way to go to keep better consistency.

For generics, that makes sens.

IjzerenHein commented 4 years ago

Actually I'm not sure what default would fit better between the simple string approach vs the reactive path. I just started to work on an application using firestorter so I still lack of experience with it to give the right answer. If you recommend usage of reactive path then we probably want to use this one. Because the library is focused on doing things automatically then it's probably the right way to go to keep better consistency.

Okay, in that case I will put this feature request on hold for now. I don't want to expand the API footprint with optimisation features that might not get used.