EddyVerbruggen / nativescript-plugin-firebase

:fire: NativeScript plugin for Firebase
https://firebase.google.com
MIT License
1.01k stars 441 forks source link

Firestore - Paginate data with query cursors #1469

Closed milansar closed 5 years ago

milansar commented 5 years ago

As Per Firestore readme Paginate data with query cursors is supported and it should work same as document on Web

It appears Plugin only works with document snapshot to define query cursor , even example given in readme is for document Snapshot. A simple cursor to a query is not working Example citiesRef.orderBy("population").startAt(1000000)

When as argument anything other than snapshot is given application throw error "Provided snapshot must not be null.". This happen because plugin assume that snapshot is provided and try to convert that to native snapshot on this line

firebase.firestore.startAt = (collectionPath: string, snapshot: DocumentSnapshot, query: com.google.firebase.firestore.Query): firestore.Query => {
  return firebase.firestore._getQuery(collectionPath, query.startAt(snapshot.android));
};

as JavaScript does not have overloaded methods , tried to change directly generated javascript file with following code

firebase_common_1.firebase.firestore.startAt = function (collectionPath, snapshot, query) {
    return firebase_common_1.firebase.firestore._getQuery(collectionPath, query.startAt(snapshot.android ? snapshot.android :  snapshot);
};

This gives Following error

java.lang.Exception: Failed resolving method startAt on class com.google.firebase.firestore.Query System.err: at com.tns.Runtime.resolveMethodOverload(Runtime.java:1245)
System.err: ... 20 more

Looking into Firebase Android-SDK code and official documentation, it is clear startAt has two overloaded methods with different signature

Not sure why it is not able to resolve to second signature when value of snapshot is just string What is correct way to implement it in plugin so everyone can get benefit out of this?

Any help on this Highly appreciated

EddyVerbruggen commented 5 years ago

The plugin was created to use a snapshot as you found out. I've never tested your kind of usage, and perhaps those overloaded methods were even added after I implemented this. So this is a feature request. Are you willing to attempt doing a PR?

milansar commented 5 years ago

I am for than happy to to attempt doing a PR, at this point I am just stuck on why overloaded method is not being called when I am not passing snapshot

Just to give you some background on why I feel second overloaded method of startAt is more important then snapshot.

Second overloaded method is required for Geo query (near by, radius search etc...) using GeoHash implementation. I guess doing near by search should be very common use case.

I cam across this issue as I tried to use GeoFirestore-JS , This library is intended for web use but with only one tweak I could make it compatible with the plugin which is great :-). Only problem is its near function depend on startAt second overloaded method.

I checked second overload method is available since 2018 and in plugin typing even it is defined as Native.Array.

Problem is android run time is not able to resolve proper method , if I go by above type definition and pass value as array, get error

Error: Cannot convert array to Lcom/google/firebase/firestore/DocumentSnapshot; at index 0

and when i just pass it as String , android run time try to compare type and it does not resolve to second overload due to different type of param of method vs what is passed as argument ( [Ljava.lang.Object; vs java.lang.string)

So it appear as Marshaling issue with Android run time and right now do not have clue how to pass argument so it satisfy need of [Ljava.lang.Object;

milansar commented 5 years ago

@EddyVerbruggen Update: I figured Marshaling issue and changed code for Android. Now it works for Android. here is snippet of hacked generated javascript code

firebase_common_1.firebase.firestore.startAt = function (collectionPath, snapshot, query) {
    if(snapshot.android) {
        return firebase_common_1.firebase.firestore._getQuery(collectionPath, query.startAt(snapshot.android));
    } else {
        const list = Array.create('java.lang.Object',1);
        list[0] = new java.lang.String(snapshot);
        return firebase_common_1.firebase.firestore._getQuery(collectionPath, query.startAt(list));
    }    
};

Now I understand what needs to be done to make it compatible with Geforce-JS library. I will make changes and create pull request after testing for ios as well

let me know if you have any thoughts on how it should be implemented

EddyVerbruggen commented 5 years ago

Hi @milansar I don't mind too much about a hacky solution as long as it works reliably.

For iOS the situation is much easier; there's queryStartingAtDocument (which I currently use) and queryStartingAtValues (which you'll need).

Btw, similar implementations are possible for startingAfter, endingAt, etc.

Happy to merge a PR!

Happy to merge a PR!

milansar commented 5 years ago

Thanks @EddyVerbruggen

I figured that. in end ios was much easier to support compare to Adnroid ;-). I was scared with ios

created PR

Thanks for awesome plugin

EddyVerbruggen commented 5 years ago

Yeah as a guy with a Java background I can relate, but I usually have an easier time with iOS as well ;)

Thanks a lot! I'll try to review and merge it tomorrow.

EddyVerbruggen commented 5 years ago

@milansar Thanks a lot! I didn't test the new features, but after a little fix on Android the old implementation works nicely. So I'm happy and I'll release this in a second.

milansar commented 5 years ago

@milansar Thanks a lot! I didn't test the new features, but after a little fix on Android the old implementation works nicely. So I'm happy and I'll release this in a second.

Thanks @EddyVerbruggen ,

I figured issue I created. Somehow after doing refactor I missed to test original function on Android hence broken original function. Sorry about that

EddyVerbruggen commented 5 years ago

No worries mate, it was an easy fix. Thanks for your contribution!

milansar commented 5 years ago

Thanks @EddyVerbruggen

created another pull request, not so much of functionality change but code improvement in terms of readability and kind of support all types supported by fire store

should integrate when you get time