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

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

Conversation

damonmaria
Copy link
Contributor

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 added a commit that referenced this pull request Oct 16, 2019
…y with `computedRequiresReaction`

Also see PR #81
@IjzerenHein
Copy link
Owner

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
Copy link
Contributor Author

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

@damonmaria damonmaria closed this Oct 16, 2019
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.

2 participants