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

Update to firebase 7.3.1 and split up targets #14

Merged
merged 1 commit into from
May 10, 2021

Conversation

grangej
Copy link
Contributor

@grangej grangej commented Dec 26, 2020

  • Updated firebase to latest release
  • Split up packages so we don't have to include all of firebase if we only want a certain piece

@alexfringes
Copy link

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

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

grangej commented Dec 27, 2020

@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
Copy link

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
Copy link
Collaborator

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

jondwillis commented Jan 23, 2021

@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
Copy link
Collaborator

@jondwillis its "everything" umbrella

@kshivang kshivang merged commit 33f7692 into urban-health-labs:master May 10, 2021
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.

5 participants