brianegan / flutter_architecture_samples

TodoMVC for Flutter
http://fluttersamples.com/
BSD 3-Clause "New" or "Revised" License
8.74k stars 1.71k forks source link

MobX example #164

Closed pavanpodila closed 4 years ago

pavanpodila commented 4 years ago

@brianegan The tests are all passing. Seems like a glitch here in the CI ?

brianegan commented 4 years ago

Thanks @pavanpodila! I'm taking a look and should have a review done in the next day or so :)

pavanpodila commented 4 years ago

Awesome! Look forward to it @brianegan

brianegan commented 4 years ago

Heya @pavanpodila -- this is really cool overall! Thanks for sharing :)

I think what you've got is a really good start, but feels a bit incomplete in some regards when looking at the app spec.

I wanted to play with MobX a bit more, so I'm working on adding some missing features and will push up the changes shortly for your review.

pavanpodila commented 4 years ago

Thanks Brian! Let me know what is incomplete. I'm happy to take it up. Of course, I won't take away the joy of using MobX from you πŸ˜„

brianegan commented 4 years ago

Yah, had a few hours of fun :) Just pushed up #166 -- please let me know what you think of the changes!

codecov[bot] commented 4 years ago

Codecov Report

Merging #164 into master will increase coverage by 1.43%. The diff coverage is 64.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #164      +/-   ##
==========================================
+ Coverage   67.33%   68.77%   +1.43%     
==========================================
  Files         161      166       +5     
  Lines        3576     3728     +152     
==========================================
+ Hits         2408     2564     +156     
+ Misses       1168     1164       -4
Impacted Files Coverage Ξ”
frideos_library/lib/blocs/stats_bloc.dart 60% <ΓΈ> (ΓΈ) :arrow_up:
mvu/lib/edit/view.dart 0% <ΓΈ> (ΓΈ) :arrow_up:
frideos_library/lib/blocs/todos_bloc.dart 89.55% <ΓΈ> (ΓΈ) :arrow_up:
mobx/lib/details_screen.dart 0% <0%> (ΓΈ)
mobx/lib/edit_todo_screen.dart 0% <0%> (ΓΈ)
mvi_flutter/lib/dependency_injection.dart 0% <0%> (ΓΈ) :arrow_up:
blocs/lib/src/todos_list_bloc.dart 79.48% <100%> (-0.52%) :arrow_down:
mvi_base/lib/src/mvi_stats.dart 100% <100%> (ΓΈ) :arrow_up:
mvi_base/lib/src/mvi_todos_list.dart 84.21% <100%> (ΓΈ) :arrow_up:
blocs/lib/src/todos_interactor.dart 81.08% <100%> (+0.52%) :arrow_up:
... and 75 more

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update cac5aff...513ea2f. Read the comment docs.

brianegan commented 4 years ago

Thanks again! I've merged all of these into the updated version which is now merged into master :)

pavanpodila commented 4 years ago

Awesome Brian! Thanks for cleaning up lot of the code πŸ‘. Curious to hear if you had any pain points using MobX :-)

brianegan commented 4 years ago

Absolutely -- thank you!

Overall, the experience was really nice. In the end, this sample is one of the smallest in terms of LOC (minus generated files), was really easy to test, and I'm excited by the extension methods on Streams and how easy those are to now interop with the Extensions provided by RxDart -- I tried my small "Github Search App" from the RxDart repo with Mobx and it was as easy as using all the normal operators then popping an asObservable at the end of the chain. Way cool!

My biggest pain points:

  1. No toString, == and hashCode generation. Those are really important for testing and I think they should be generated automatically -- I had to generate them using IntelliJ.
  2. The way the Store is defined makes adding custom constructors pretty painful. For example, I wanted to add the toEntity method and fromEntity static method to the Todo class. It was easy enough with the toEntity, but adding fromEntity was not fun (redirecting constructor, etc). In the end, I went with the Codec instead. I'm sure you've tried many options for how to make the code generation work and found this was the best approach, but it's definitely a pain point.
pavanpodila commented 4 years ago

Thanks Brian!

For 1. we discussed about auto-generating that and will take it on priority. Not sure if there is a quick and easy way to generate hashCode though.

For 2. this is the best we could do in terms of codegen and still allowing extension from the outside. We explored the use case of generating a public class in the codegen but ran into lots of issues with extensibility. For now this is the best compromise. I am sure this will improve as Dart gets better facilities and language features.