search

urban-health-labs / CombineFirebase

Combine wrapper on Google's iOS Firebase library.
MIT License
225 stars 44 forks source link

Update to firebase 7.3.1 and split up targets #14

Closed grangej closed 3 years ago

grangej commented 3 years ago
alexfringes commented 3 years ago

This was super useful. I was just about to fork in order to update the FB version, but the splitting is a nice bonus. Seems like it would be great to get this merged to allow new users to adopt easily with SPM? Otherwise SPM support is kind of going to waste, especially since the FB team has put so much extra work in since the version that's currently targeted, so I don't see people going the route of locking into that old version if they are using SPM.

alexfringes commented 3 years ago

One odd effect of importing CombineFirebaseFirestore, for example, instead of CombineFirebase, was a bunch of warnings along the lines of "No calls to throwing functions occur within 'try' expression" across the project even in files that aren't importing CombineFirebase / CombineFirebaseFirestore. Switching the import statement to just CombineFirebase seems to resolve it. I ended up not going the selective route on the dependencies, is it possible that in that case I shouldn't use selective import statements? If so, that might require some extra work to prevent that error, or clarification in the docs, if this is merged.

grangej commented 3 years ago

@alexfringes Interesting, I haven't tried the firestore one yet, I was using only auth at the moment in my project and it seems to be working ok so far. I will try the firestore module and see why this fails.

alexfringes commented 3 years ago

This is actually my first time checking out CombineFirebase (due to the SPM issue) and I ran into a bunch of errors with the sample code while trying to use the Firestore component, so I might put adopting this on a brief hold, but will keep an eye on this thread. If you do get it to work with Firestore code, I'd be curious to hear if you're following the ReadMe's sample code or adapting it somehow.

kshivang commented 3 years ago

NIce work @grangej . Was busy last couple of weeks. Will review this PR by Sunday. @alexfringes thanks for pointing out firestore import problem will look into it. Meanwhile any updates(or review) on this PR is very much appreciated.

jondwillis commented 3 years ago

@grangej Awesome work.

One question that I had while trying this out:

What is the difference between adding the CombineFirebase package versus the other packages e.g. CombineFirebaseAuth? Is CombineFirebase an "everything" umbrella that imports all of the others, or is it more like a shared "Core"?

Also, Firebase updated to 7.4.0 while this PR has been hanging out.

kshivang commented 3 years ago

@jondwillis its "everything" umbrella