TobiasBuchholz / Plugin.Firebase

Wrapper around the native Android and iOS Firebase Xamarin SDKs
MIT License
220 stars 49 forks source link

[2.0.0] Firestore DictionaryExtensions change #152

Closed tranb3r closed 1 year ago

tranb3r commented 1 year ago

Hi,

I'm having trouble upgrading Firestore to version 2.0.0. The DictionaryExtensions (for android) have changed a bit compared to previous version. I was previously using a Dictionary<object, object> when setting data in Firestore document, but the ToHashMap extension is now expecting a IDictionary<string, object>, so another extension is used instead (the one using an object in input), and it does not work: no exception, no error message, but the data is not set properly in Firestore.

Is there any reason for this change? Should we update the code in our apps? Or can you bring back the previous behavior?

Thanks!

TobiasBuchholz commented 1 year ago

In the process of splitting up the features into separate packages I have removed all methods that had no references, so the ToHashMap method you are referring to must have been one of it. If it's possible you should update your code accordingly, otherwise let me know.

morgan-hill commented 1 year ago

I'm having problems relating to NSDictionary and converting to Firebase.Core.Options type in CrossFirebase.Initialize for ios and android.

I have question in stackoverflow.

https://stackoverflow.com/questions/75904143/plugin-firebase-maui-errors-cannot-convert-argument-on-crossfirebase-initialize

TobiasBuchholz commented 1 year ago

The setup instructions in the documentation had a mistake, the CrossFirebase.Initialize method doesn't require any arguments on iOS and only an Activity on android, so your code should actually look like this:

#if IOS
            events.AddiOS(iOS => iOS.FinishedLaunching((app, launchOptions) => {
                CrossFirebase.Initialize();
                return false;
            }));
#else
            events.AddAndroid(android => android.OnCreate((activity, state) =>
                CrossFirebase.Initialize(activity)));
#endif
        });

The mistake in the readme got fixed accordingly.

tranb3r commented 1 year ago

Look at the signature of the SetDataAsync method: https://github.com/TobiasBuchholz/Plugin.Firebase/blob/8dab85b15cb66fafd35c99b8b4806005f4c7eb6a/src/Firestore/Shared/IDocumentReference.cs#L24

It's taking a Dictionary<object, object> in input. So it makes no sense to remove the corresponding ToHashMap extension.

The current code compiles because there is another extension (the one using an object in input). Maybe this is why you thought that the extension had no references, when you did the split. But now the code is not working at all for a Dictionary<object, object>.

So, I think this really needs to be fixed, and I don't think it would be a good idea to try to workaround this issue by modifying the code on client side. What do you think?

Here is the code to re-add:

  public static HashMap ToHashMap(this IDictionary<object, object> dictionary)
  {
      var map = new HashMap();
      dictionary.ToList().ForEach(x => map.Put(x.Key, x.Value));
      return map;
  }

I can prepare the PR if you want, just let me know. Thanks!

TobiasBuchholz commented 1 year ago

Ah ok, I see. Yes, a PR would be much appreciated! :)