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

Migrate to v7: More complete example #2962

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Migrate to v7: More complete example #2962

wants to merge 3 commits into from

Conversation

mparpaillon
Copy link

connectEmulator does not exist. It's connectFirestoreEmulator. Also I've added an example for all the others. Including auth which has different arguments (port is included in the URL)

connectEmulator does not exist. It's connectFirestoreEmulator. Also I've added an example for all the others. Including auth which has different arguments (port is included in the URL)
@google-cla
Copy link

google-cla bot commented Sep 16, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Sep 16, 2021
@mparpaillon mparpaillon changed the title More complete example Migrate to v7: More complete example Sep 16, 2021
@mparpaillon
Copy link
Author

I'm having a hard time migrating to v7 to be honest. Looking into Firebase-sdk and rxfire docs to find how it's supposed to work. Hopefully this doc update will make it easier to someone

@hakimio
Copy link

hakimio commented Oct 18, 2021

I have recently migrated akita-ng-fire. The migration includes auth, firestore and database. You can see the PR here.

Some of the issues I have encountered:

  • rxfire is a mess with their function naming duplicating firestore functions and misleading documentation.
  • angularfire itself adds extra layer of confusion by reexporting rxfire functions with different names and, in some cases, proposing substitutes which are not direct substitutes in its migration guide. For example, docSnapshots() is not a good substitute for snapshotChanges() since docSnapshots() will emit twice as many values compared to snapshotChanges because includeMetadataChanges is set to true in that one. The correct subtitute should be poorly named and undocumented fromRef().

@jamesdaniels
Copy link
Member

Documentation is pending. @hakimio include metadata changes was enabled in AngularFire v6, I don't think the behavior is dramatically different as rxfire was a copy/paste from angularfire. If you're having troubles feel free to remain on compat, it will be supported for some time.

rxfire has duplicate naming because of legacy reasons, it was pure functions before JS SDK was. It was prior art in the new API design.

As always this is an open source project & only one of my many responsibilities; please keep it constructive and we welcome pull requests.

@hakimio
Copy link

hakimio commented Oct 19, 2021

@jamesdaniels
Thank you for your work and sorry if my comment seemed mean.
Just a couple of related questions:

  • Why metadata changes are not optional in angularfire like they are in firebase-js-sdk?
  • Wouldn't it be better to rename functions in rxfire instead of reexporting them with a different name in angularfire?

@jamesdaniels
Copy link
Member

@hakimio I didn't take it as mean, just saying that we're open source, time constrained, and we welcome help. In general I think a lot of folk think that these libraries are well staffed and that we have all the resources in the world, but even over on our side of the fence this is a volunteers-only project.

A) At the time this was designed, we didn't have configuration that could be passed into snapshotChanges, ultimately we decided that fromRef existed for more advanced cases and that one could pair down emissions with distinctUntilChanged etc. These "extra" emissions cost you nothing extra. It's much harder to explain to folk why they aren't seeing emissions they want, rather than explaining how to filter. In v5 were getting a lot of complaints that metadata changes weren't included, so we addressed in AF v6 & found there were some missing cases and fixed in a minor. In RxFire-land we didn't land this PR in time to cut with rxfire v6, we can probably sneak it into a minor.

B) that would be a break & would require a major, changing docs, confusing people on older versions; ultimately we didn't have time to do that (as I was on paternity leave) before JS SDK v9 dropped. We had a mad rush to move over all the docs, cut a bunch of libraries, are still playing wack a mole with edge cases, while also trying to find time to take some time off so we don't burn out 😝 I consider rxfire a dependency of AngularFire still at this point (rather than a peer) and don't care if we diverge, I'd rather developers not have to consider our internal implementation. This is only problematic right now while we're working on the AF docs.

If you see any bugs with docs or implementation please file issues and we welcome PRs.

@google-cla google-cla bot added cla: yes and removed cla: no labels Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants