IjzerenHein / firestorter

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

Allow hasDocs when mode = "off" and mobx computedRequiresReaction #81

Closed damonmaria closed 4 years ago

damonmaria commented 5 years ago

As the hasDocs getter was computed it would cause a mobx error when called outside of a reaction (when collection.mode === Mode.Off) and mobx was configured as computedRequiresReaction. So this now makes a separate internal computed property _hasDocsComputed which is called in reactive modes, otherwise it calculates it directly.

I'd prefer _hasDocsComputed was declared private in TS but that doesn't work in the decorate call :(

There is some other reformatting committed thanks to Prettier.

I've also committed wallaby.js for running these tests with Wallaby (which I'd highly recommend for anyone doing unit testing in JS). Normally Wallaby does not need a config file but since the tests all share a single DB they can't be run in parallel so I have to disable that. Have you considered switching to the Firestore emulator? It's quite easy with that to create a separate DB for each parallel test run. Testing would be much faster.

IjzerenHein commented 4 years ago

Hi! I've taken a closer look at your PR and have decided to solve the hasDocs problem a bit differently. I've changed the code to have a separate observable which keeps track of whether there any any docs or not. This is slightly more efficiently, but more importantly it makes hasDocs no longer computed, which resolves the problem aentirely. I've released this as v1.2.3.

As for the other changes in the PR, I've decided not to pull any of them in. Any styling changes auto-fixed by prettier should always be a separate PR, as well as the Wallaby addition. Personally I used and tried wallaby, but I'm not a fan :)

As for the suggestion to run the tests using the Firestore emulator, that's really a great idea! I've created a separate issue for that #83

cheers man!

damonmaria commented 4 years ago

Totally agree with all this. Since docsObservable is modified in just the one place it's actually super simple.