-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
feat(flutter_todos): Add firestore backend. Support compile time API selection. #3510
base: master
Are you sure you want to change the base?
Conversation
HI there ! I was just awaiting feedback on the tests that we implemented |
Hey @Gene-Dana I'm happy to collaborate on providing feedback on existing tests, and implementing todos_api_factory tests. I will be getting to that obviously; however given that I still have some millage to cover in my Dart & Flutter learning I believe it will take me a week or 2 before I can close the loop on that. Happily accepting contributions! |
Also, there is an issue with the ordering. In the original local storage API there the todos are stored in a list and the orders is preserved. In Firestore, this is not the case, and so I simply added a timestamp as the ID. Not sure this is the worlds best implementation and was looking for feedback regarding this.
What do you mean by this? |
Hi there 👋🏼 hoping everything is okay. Haven't received any feedback on the way that the tests were written ! |
Hey there @maximveksler just checking in if there was any feedback you or @felangel could offer |
Adding help wanted here would be great. I do see myself getting back to working on this in the forseeable future, however would be glad to push it forward. Implementation is DONE, test coverage is lacking. |
@felangel reviews please ! |
Hi there @maximveksler, I'm seeking to understand your approach with the Originally when I spoke with @felangel about implementing this he suggested that we created separate import 'package:cloud_firestore/cloud_firestore.dart';
import 'package:firebase_core/firebase_core.dart';
import 'package:firestore_todos_api/firestore_todos_api.dart';
import 'package:flutter_services_binding/flutter_services_binding.dart';
import 'package:flutter_todos/bootstrap.dart';
import 'package:flutter_todos/firebase_options.dart';
Future<void> main() async {
FlutterServicesBinding.ensureInitialized();
await Firebase.initializeApp(options: DefaultFirebaseOptions.currentPlatform);
final todosApi = FirestoreTodosApi(firestore: FirebaseFirestore.instance);
bootstrap(todosApi: todosApi);
}
Is there any distinct advantages to the factory besides reducing the number of mains? I thought @felangels approach was intuitive Also, for the sake of instructing others by example I thought the original approach made more sense |
Hey @Gene-Dana I belive that the approach of having several The usage of a single main seems to be more generally suitable case for introduction into real life applications. Therefore I've used the The update in this PR is introduced with the default being the current behavior (i.e. local storage), which allows me to introduce the change in a backwords compatible fashion, for the Please let me know if that ansawers you question, happy to have the solution architecture discussion followups. |
Status
WIP Looking for help in implementing unit test coverage.
Breaking Changes
NO
Description
Adding Firestore support into the Todos example.
#2859 introduced a Well-Architected todos example, however in doing so dropped Firestore support.
A comment by @jeroen-meijer shows that there is interest by the community to see an elegent Firestore integration in the todos example.
This PR implements the requirment of no code duplication, leaving the choice of which implementation ot use to the developer.
local storage
remains the default out of the box implemenation.firestore
usage is documented indocs/firestore/README.md
packages/firestore_todos_api
is cherry picked from the work done by @Gene-Dana in feat/flutter-todos-firestore branch.Type of Change