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

Add listDocuments to collection #133

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

op
Copy link

@op op commented Mar 1, 2019

This adds support for the listDocuments method to CollectionReference.

I need this when calling firestore from a service and not from a client. When preparing this PR I realised that this method only exists in the firebase-admin package and not in the firebase package. How have you dealt with differences like this before?

References:

  1. https://cloud.google.com/nodejs/docs/reference/firestore/latest/CollectionReference#listDocuments
  2. https://firebase.google.com/docs/reference/js/firebase.firestore.CollectionReference
  3. Add List() API googleapis/nodejs-firestore#368

@joelpoloney
Copy link

Bump. Can we get this merged in?

@dmurvihill
Copy link

dmurvihill commented Nov 19, 2019

Hi @joelpoloney, this repo is no longer used (see #160) and we will be publishing new releases from my fork until we land on a permanent way forward. I've opened a new PR over there with this change.

We are also looking for maintainers.

@joelpoloney
Copy link

Oh! So sorry to hear :( Thank you for the update! I ended up forking it and support a few other use-cases as well: https://github.com/timbersoftware/firebase-mock/commits/master. Once your fork becomes the new source of truth, I can submit some PRs to you with the changes I made.

@dmurvihill
Copy link

dmurvihill commented Nov 19, 2019

Thanks, I'd be very grateful. I should become owner of the NPM name on or about 1 December and I'm looking to publish a minor version very soon after. So if you can open any PRs before next Thursday (Thanksgiving in the U.S.), that would give you the best chance of getting into the release.

We're all very grateful, I'm sure, for the quality package Bryan left us. Marrying all of our different forks and carrying on his work would be a great way to say thank you.

@joelpoloney
Copy link

Ok great! I can try to open the PRs for you before Thanksgiving. Some are easy to open... others are me hacking away at the code to fix a bug but probably isn't the best long term fix.

Copy link

@dmurvihill dmurvihill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to require some doing.

As @op pointed out, this API is only available through firebase.admin.firestore and not through firebase.firestore. We need to test the different import paths and make sure they each have the correct behavior.

I've been looking at our code base and it looks like we don't actually have any example yet of firebase.admin imports behaving differently from firebase imports. One idea would be to extend the prototype in the admin package and export it with the same name.

Any ideas?

db.collection('group').listDocuments().then(function(refs) {
expect(refs.length).to.equal(1);
refs[0].get().then(function(doc) {
expect(doc.data().name).to.equal('test');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just go all the way and check for deep equality here.

expect(keys).to.contain(ref.id);
});
done();
}).catch(done);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this going to silently swallow exceptions that occur before any assertions are run and give us a false pass?

Copy link

@SidneyNemzer SidneyNemzer Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Mocha's done accepts an error argument, so this should work as intended. https://mochajs.org/#asynchronous-code

Copy link

@dmurvihill dmurvihill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're going to punt the issue of admin versus regular SDK to version 4. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants