search

sbatson5 / firestore-jest-mock

Jest Helper library for mocking Cloud Firestore
https://www.npmjs.com/package/firestore-jest-mock
178 stars 59 forks source link

Subcollection Mocking #35

Closed AverageHelper closed 4 years ago

AverageHelper commented 4 years ago

Description

Here we add full support for subcollection mocking. To do this, we needed to restructure things a bit, change what exactly our mocked functions return, etc. This way, we no longer get TypeErrors when trying to call typical Firestore API under our mock.

See the new paragraphs under the README for the syntax of this feature.

Related issues

Fixes #25

How to test

jest

I've already tested this fork in a production project, and we're happy enough with it. There are a couple more ways I'd like to be able to assert Firestore accesses, but this will do for now.

AverageHelper commented 4 years ago

I agree it's a lot, but I have no idea how to split it up as-is. I'd be happy to go over it with you in some way if you'd like. I tried to break the big bits into their own files to make things easier. With my limited JavaScript experience, however, and with some of these parts depending on each other tightly to work properly (thinking of CollectionReference and DocumentReference each needing the other to be defined before their own internals can work), I don't know how to break it up any further. 😕

If you mean that I should just submit a few commits at a time in separate PRs, I'm afraid I have no idea how to do that with Git while keeping up-to-date with master. My last several commits are just rebasing master's changes onto mine to make everything play nicely.

sbatson5 commented 4 years ago

👋 @AverageHelper

I'm happy to spitball how we can break this up. Not only will it make reviewing easier, but it will be better for any future contributors who want to look for when a particular feature was added.

The first thing that jumps out at me are the changes to auth:

Next, I think you could move the Query logic to its own PR without breaking much. The only thing it depends on is buildQuerySnapShot, so it could be a good opportunity to move that and buildDocFromHash to helpers in that PR.

After that, Transaction feels pretty isolated as well (it has no dependencies and you have it well tested).

Then, the changes you made to Timestamp could be a separate PR. Followed by FieldValue, which depends on it.

After that, I think CollectionReference could be its own PR as it depends on the Query and a few other things. The only hurdle is not using the DocumentReference, so that might have to be in the same PR. This would probably be the biggest change, but it would still be much easier to parse.

In terms of digging through your commits, I wouldn't worry about maintaining that history. I squash the commits when a PR gets merged to maintain a cleaner history (if you dig through our commit history, you'll see each commit ties to a concise PR/feature). So, I would simply copy and paste the files in question. The history of when certain lines were changed during development isn't as important as when they were merged.

AverageHelper commented 4 years ago

Will do.

AverageHelper commented 4 years ago

The first thing that jumps out at me are the changes to auth:

  • getUser doesn't depend on anything
  • setCustomUserClaims doesn't depend on anything

@sbatson5 See #36

AverageHelper commented 4 years ago

I've got most of the minor changes done in #36, #37, #38, #39, and #40, in that order. I think I should probably wait until those are all merged and ready before I do the whole subcollection support bit. That makes it easier on me in case there are any changes y'all want on one of these smaller PRs.

While subcollection support would be a fairly major change, the API surface shouldn't change for those who don't rely on FakeFirestore having a few more methods than canon firestore (e.g. FakeFirestore.get would need to go, or be deprecated with some dummy implementation). We'd have a few more files and more granular access to certain mock structures, but most everything still exports publicly via FakeFirestore, following Firestore's convention.

AverageHelper commented 4 years ago

Now that we have subcollection support, I have a few ideas on how we might assert access to document paths, since assertions on mockCollection or mockDoc's arguments and NthCalledWith reports quickly become unwieldy with complex Firestore schemas. Expect an issue from me sometime in the next week or two about this. See #52.