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

Firestore.js Housekeeping #40

Closed AverageHelper closed 4 years ago

AverageHelper commented 4 years ago

Description

This one just moves Timestamp and FieldValue into their own files. The classes remain exported through FakeFirestore and firestore, while their mock functions are exposed through firestore.js. API surface shouldn't change much at all, and existing code should still work.

Related issues

Depends on #39

How to test

jest
FrozenKiwi commented 4 years ago

Note to self: when this PR is submitted, I have another one to add direct doc & collection access via path at https://github.com/thecointech/firestore-jest-mock

AverageHelper commented 4 years ago

@FrozenKiwi Same here. I've another PR in the works that depends on this one, #38, and #39 first.

Here's my implementation. I'd love some pointers!

See also #35, which will be cannibalized into that new PR, once finished.

FrozenKiwi commented 4 years ago

I figured you'd have that already, but I had already merged your PR's locally and figured I'm not patient enough :-)

My implementation is here. Pretty much the same thing. I do like your coding style though - it looks very nice and clean. I seem positively shabby by comprison LOL (that's my C roots coming through though).

But I would recommend extracting that iteration out into a function. It is legal to directly access a collection by path as well (which requires the same iteration). Also, your code will fail silently(?) on an invalid path, whereas I just realized mine will (wrongly) succeed (with the wrong result). Both approaches are probably wrong, we should probably throw if the path has an even number of elements. The unit-tests I used to validate are here; https://github.com/thecointech/firestore-jest-mock/blob/9fe337ee8ac71b91f2909798f7553567b207bf3f/__tests__/mock-firestore.test.js#L90. There probably should be one with an invalid path to test that scenario. Rather find out on this DB than the live one.

Side note: if you haven't yet, I've also added in a function to transform timestamps in the incoming database. https://github.com/thecointech/firestore-jest-mock/blob/9fe337ee8ac71b91f2909798f7553567b207bf3f/mocks/firestore.js#L203. In my unit tests I like to keep my sample data as json files.

FrozenKiwi commented 4 years ago

By the way, I think your awesome @AverageHelper, and Upstatement too. Peeps like you make the software world go around, and we are all so much better off because of it. Someone who find's a problem and just goes ahead and fixes it. (And takes the time to do it right, so others don't hit the same problem). Sending virtual, appropriately-socially-distanced high-fives!

AverageHelper commented 4 years ago

Thank you, kind person! I'll be sure to go over that block again soon, before it becomes a PR. I agree it could certainly be done better, and I agree that extracting the loop to a separate function would help the code's readability. More unit tests would certainly help, too. All great points!