Skip to content

Add Lint checks for FirestoreRecyclerAdapter lifecycle methods #1339

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

Closed
pavlospt opened this issue Jun 3, 2018 · 4 comments
Closed

Add Lint checks for FirestoreRecyclerAdapter lifecycle methods #1339

pavlospt opened this issue Jun 3, 2018 · 4 comments

Comments

@pavlospt
Copy link
Contributor

pavlospt commented Jun 3, 2018

Describe the problem:

Currently there are no Lint checks on whether someone has called .startListening() / .stopListening() methods when using a FirestoreRecyclerAdapter, or .setLifecyclerOwner() when using FirestoreRecyclerOptions.Builder. That way the RecyclerView using that adapter will never get any UI updates on items from real-time changes.

Steps to reproduce:

Create a FirestoreRecyclerAdapter and do not call .startListening() / .stopListening() methods.

Alternatively, use FirestoreRecyclerOptions.Builder and do not call .setLifecycleOwner().

Observed Results:

RecyclerView using the adapter will not get any UI updates from real-time changes without any obvious reason, for those who haven't called .startListening() or .setLifecycleOwner().

Expected Results:

Expected a Lint check in place, to inform on whether there is a missing call on .startListening() or .setLifecycleOwner() for FirestoreRecyclerOptions.Builder.

Relevant Code:

I have prepared a relevant PR: #1340

@SUPERCILEX
Copy link
Collaborator

I like this idea, but I have trouble seeing how it will scale. People might be passing the builders around or managing lifecycle in another class. Basically, I feel like this issue is much more easily addressed by making our documentation clearer since there are tons of APIs out there that require something to kickstart them, us included.

Anyway, that's my initial reaction. @samtstern What do you think? If we do go forward, we'll have to move some modules around since our lint checks were meant to be internal.

@samtstern
Copy link
Contributor

@pavlospt first of all thanks for the very interesting suggestion and the extremely well-done PR.

My main concern here, as @SUPERCILEX said, is that we are not currently shipping any external lint checks. So it would require some module shuffling, probably the creation of a firebase-ui-lint artifact or similar.

@pavlospt
Copy link
Contributor Author

pavlospt commented Jun 6, 2018

Hey, @SUPERCILEX & @samtstern thank you for your opinions on the matter! I am open to any structural suggestion on the project and would be more than happy to contribute more once we end up with a proper solution :) A separate artifact sounds reasonable as well! I can hold off the PR and commit those checks once the structural changes have been applied to the project!

Also, thanks to Juhani Lehtimaeki (https://twitter.com/lehtimaeki) for finding the need of those Lint checks and to Stratos Theodorou (https://twitter.com/eeVoskos) for suggesting to turn those into Lint checks!

@samtstern
Copy link
Contributor

Closed by #1340

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

No branches or pull requests

3 participants