search

IjzerenHein / firestorter

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

Add "context" option to avoid global state #60

Closed justjake closed 5 years ago

justjake commented 5 years ago

This PR adds a context: IContext option to the constructors of Document and Collection. An object's context contains references to its Firebase app and Firestore instance, which will be used instead of the global references in init.ts. Collections propagate context to their child documents.

This allows consumers to use Firestorter with multiple Firebase apps, which is especially helpful when writing tests using the '@firebase/testing' module.

No changes are needed to existing code: if context is undefined on a Document or Collection, the global context will be used instead.

I haven't added tests for the new context option yet: I first wanted to ask for feedback on this approach. Does this seem like a useful feature, @IjzerenHein?

I'm hoping to test my Firestorter code against the Firestore simulator following this testing pattern

justjake commented 5 years ago

I think the failing tests are failing on master, too, so that's not a regression.

IjzerenHein commented 5 years ago

Hey Jake, this looks very good, thanks for the PR! This is very good feature to add, I will review and merge when I have a bit more time, probably next week.

As for the tests failing, that's correct, there is one new test added that exposes an issue that a user witnessed. I'll probably disable that test for now until the actual problem gets fixed.

cheers, Hein

IjzerenHein commented 5 years ago

Hey man, sorry for not getting around to merge this just yet. I'm planning to do this tomorrow, I'll let you know in case I have any questions. Cheers, Hein

justjake commented 5 years ago

Don’t feel any pressure to merge this yet! I haven’t added unit tests to my branch or used it in my project yet – plus it’s the holidays. Take it easy :)

I’ll ping you again when I think it’s ready to merge.

IjzerenHein commented 5 years ago

Alright cool, just let me know :) Happy Holidays dude

justjake commented 5 years ago

@IjzerenHein I haven't tested context moving from a Collection to its Documents, but I've been running this code on my site for a bit and it seems solid.

IjzerenHein commented 5 years ago

Sweet, I'll have a closer look at it shortly!

IjzerenHein commented 5 years ago

Hi, it's taken too long, but I've finally merged this PR and released a new version. Thanks for this contribution Jake. cheers, Hein