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(database): migrate to paging 3 #1983

Merged
merged 6 commits into from
Jul 30, 2021

Conversation

thatfiredev
Copy link
Member

@thatfiredev thatfiredev commented Jul 29, 2021

This PR should:

  • update the database module to use Android Jetpack's Paging 3
  • update the sample app to use Paging 3
  • update the docs

See #1982

@thatfiredev thatfiredev mentioned this pull request Jul 29, 2021
7 tasks
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.

Approve, just one comment you may want to address before I merge. Thank you for this!

}

return Single.fromCallable(() -> {
Tasks.await(task);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will throw if the Tasks fails:
https://developers.google.com/android/reference/com/google/android/gms/tasks/Tasks#await(com.google.android.gms.tasks.Task%3CTResult%3E)

So we can't rely on getting to the next line. Although I think it's fine since you re-throw at the bottom, but I just don't think you'll ever get that far.

Also we can probably remove the task.isSuccessful nesting since we know if we even get to the next line, the task is successful.

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 Interesting, I didn't know that.

I have wrapped it around a try/catch so that developers using FirebaseUI can get the original cause instead of await's ExecutionException.

@samtstern samtstern merged commit da7beaf into firebase:version-8.0.0-dev Jul 30, 2021
@samtstern samtstern added this to the 8.0.0 milestone Jul 30, 2021
@thatfiredev thatfiredev deleted the rpf-rtdb-paging-3 branch July 30, 2021 14:50
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