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

refactor(firestore): migrate pagination to Paging 3 #1915

Merged

Conversation

thatfiredev
Copy link
Member

@thatfiredev thatfiredev commented Feb 17, 2021

DO NOT MERGE

This PR should serve as a draft to implement Paging 3 in our Firestore module. Paging 3 is currently in beta, so we'll need to wait and only merge this PR once it hits the stable channel. I've opened this to discuss those changes and gather feedback from the community. Once approved I can replicate the changes on the RTDB module.

Deprecated/Removed classes and methods

Only listing code used by FirebaseUI. The Paging 3 library might have more deprecations that are not relevant to our library, therefore not mentioned here.

From the Paging Library

Deprecated Replacement
PagedList.Config PagingConfig
PagedListAdapter PagingDataAdapter
PageKeyedDataSource PagingSource
PagedList<T> PagingData<T>

From FirebaseUI

FirebaseUI 7.x FirebaseUI 8.x
FirestorePagingOptions.setQuery(Query, PagedList.Config, ...) FirestorePagingOptions.setQuery(Query, PagingConfig, ...)
FirestorePagingOptions.setData(LiveData<PagedList<DocumentSnapshot>>) FirestorePagingOptions.setPagingData(LiveData<PagingData<DocumentSnapshot>>)
FirestoreDataSource FirestorePagingSource
FirestorePagingAdapter.onLoadingStateChanged() Removed. Use the Paging 3 Load State Listener
FirestorePagingAdapter.onError() Removed. Use the Paging 3 Load State Listener
LoadingState Removed

Tests

I deleted the FirestoreDataSourceTest because all tests relied on the DataSource's loading state which has now been removed.
We might need to write new tests (with the same test cases) for our FirestorePagingAdapter, since Loading States are now exposed by it.
Update: A new test was added in #1989

@google-cla google-cla bot added the cla: yes label Feb 17, 2021
@samtstern
Copy link
Contributor

@rosariopfernandes wow thank you for putting this together and laying out the case so cleanly. I absolutely hate the way that the Android team has handled Java 8. In my view there are very few Java 8 Android devs because you're either stuck on Java 7 or you're ready for Kotlin.

You make a good case for this! It's good to know that new projects support this by default. It's a bit awkward that the official Firebase Android SDK does not require Java 8 and FirebaseUI would.

The main reason I am hesitant is because the paging adapter is a relatively niche feature of this library, so doing a big breaking change for paging users may inconvenience many others. After all the majority of usage of this library, especially in real apps, is for AuthUI. And then for users that do use the database adapters, paging is only a subset of them.

What do you think? You're more tuned in to the Android community than I am! Do a lot of non-Kotlin libraries require Java 8 these days?

@thatfiredev
Copy link
Member Author

@samtstern Thanks for outlining all the implications of Java 8, you raise some good points.

Q: Do a lot of non-Kotlin libraries require Java 8 these days?

A: TBH I don't know. I generally use Java 8 on my projects so I rarely check the library's min java version.

So from my point of view the only component requiring us to update to Java 8 is the new PagingSource so maybe we could keep the deprecated PageKeyedDataSource but update everything else. This means users of the FirebaseUI + Paging3 combination won't need to update to Java 8 and won't see many (or any at all) deprecation warnings on their IDE/builds.
I'll update this PR to give it a shot.

@thatfiredev
Copy link
Member Author

@samtstern So the plan I described in the previous comment actually worked! 🥳 The firestore module is now ready for Paging 3 and developers don't need to update to Java 8 🙂

@samtstern
Copy link
Contributor

@rosariopfernandes sorry for my slow response here! Been a little busy but will take a look at this a soon as I can. Very impressed that you managed to work it out.

Copy link
Contributor

@samtstern samtstern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if all the effort to do this in a non-breaking way (which is very impressive!) is worth it? Having a situation where setData(LiveData, Class) is deprecated but setPagingData(LiveData, Class) is supported makes it sort of hard to use the API intuitively.

Since we already have an 8.0 release on the horizon would you rather just do a hard breakin from Paging 2 to Paging 3 in that release?

@thatfiredev
Copy link
Member Author

@samtstern Thanks for taking a look at this! Yes, a breaking change from Paging2 to Paging3 in 8.0.0 sounds fine, since we don't yet have a release date for a stable version of Paging 3.

Before I address your comments on addLoadStateListener: Since we're making a breaking change, what if we remove the onLoadingStateChanged and onError methods from our adapter? The only reason why I kept them and used AddLoadStateListener was because I was trying to keep supporting Paging 2, which doesn't provide an easy way to check for loading state and/or errors.

Paging 3 on the other hand exposes a loadStateFlow property in the new PagingDataAdapter which is a Flow that allows Kotlin developers to check the loading state directly (the alternative for Java developers is addLoadStateListener). And the Paging 3 documentation does explain how to use it, so it should be fine if we let developers handle load states themselves, right?

@samtstern
Copy link
Contributor

@rosariopfernandes that all sounds great, if we can put this in 8.0 then you can do a true breaking change and use more of the Paging 3 API as it was meant to be used. I think everyone wins there!

@samtstern samtstern added this to the 8.0.0 milestone Mar 5, 2021
@thatfiredev thatfiredev changed the title refactor(firestore): add paging 3 in non-breaking way refactor(firestore): migrate pagination to Paging 3 Apr 14, 2021
@thatfiredev
Copy link
Member Author

@samtstern Paging 3.0.0 is now stable! 🎉
This PR is ready for review. :)

@thatfiredev thatfiredev marked this pull request as ready for review May 5, 2021 23:51
@samtstern
Copy link
Contributor

Version 7.2.0 was released today which means we're ready to start with 8.0.0 development including this PR!

@samtstern
Copy link
Contributor

@rosariopfernandes ready to come back to this one?

@thatfiredev
Copy link
Member Author

@samtstern Sorry for the delay, I've been a bit busy and forgot to update this PR. I've replied to your last comment just now.

@thatfiredev
Copy link
Member Author

@samtstern Do you think it would make sense to have a separate module for pagination (e.g. firestore-paging)?
This module would require Java 8 because Paging 3 requires Java 8. And we could even write it in Kotlin, since Paging 3 was entirely rewritten in Kotlin.

Developers who are still using Paging 2 / Java 7 would stick to FirebaseUI 7.x

@samtstern
Copy link
Contributor

@rosariopfernandes I am fine to actually move the entire FIrebaseUI library to Java 8 for version 8.0 ... a lot of the AndroidX libs are starting to require it and as you said developers who can't afford to adopt Java 8 can stick to FirebaseUI 7.2.0 which is quite stable.

What do you think?

@thatfiredev
Copy link
Member Author

@samtstern sounds good to me. I'll update this PR to move entirely to Paging 3 + RxJava

@thatfiredev
Copy link
Member Author

@samtstern all done and ready for review!

@@ -84,34 +83,42 @@ protected void onBindViewHolder(@NonNull ItemViewHolder holder,
@NonNull Item model) {
holder.bind(model);
}
};
adapter.addLoadStateListener(new Function1<CombinedLoadStates, Unit>() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing Function1<> 🤮 reminds me that we can probably convert the sample app to Kotlin once we go to 8.0!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely should!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samtstern actually, since we migrated to Java 8, this can actually be replaced with a lambda function:

adapter.addLoadStateListener(states -> {
    // ...
}

So we don't really need Function1<> here. 😅

@samtstern
Copy link
Contributor

@rosariopfernandes this looks great thank you for all the hard work! Really appreciate your taking the time to do the docs as well.

@samtstern samtstern merged commit b0877f2 into firebase:version-8.0.0-dev Jun 28, 2021
@thatfiredev
Copy link
Member Author

@samtstern My pleasure!
I'll start migrating the rtdb module now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants