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

initial example #2

Closed
wants to merge 1 commit into from

Conversation

rrousselGit
Copy link

@rrousselGit rrousselGit commented May 20, 2019

That's a raw example.

I haven't checked absolutely everything nor did I do a complete rewrite. I just made the changes so that things work without get-it dependency.

ProxyProvider has yet to merge, and I'd expect a good chunk of code to disappear once all the variations are available (they have a TODO(rrousselGit)).

functional_widget is totally optional. I just like it.
I'd usually add flutter_hooks too, especially to memoize event handlers. But that's not exactly important here

@rrousselGit
Copy link
Author

Note that using this architecture, classes in the model/ folder do even more stuff than before.
They now handle themselves how to fetch data.

Widgets just set some values here and there.


Future fetchComments(int postId) async {
Future _fetchComments(int postId) async {
if (state == ViewState.Busy) {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't agree with any of the Changes in the Comments Model. Especially the fact that we would now have to check the busy state and throw errors because the code might already be running. Making the fetch "automatic" in some cases would be better. But not here, I am a proponent of readable code. I know that's not a big topic, but because I write code bases for other companies it needs to be easy to understand and most importantly easy to teach. New developers coming in shouldn't have a hard time at all just looking at the code and seeing what's happening. Which is why I had so little code in this class, I don't think it should be responsible for fetching it's own data. It needs to be told what to do, from my point of view and what I want from the architecture,

Copy link
Author

@rrousselGit rrousselGit May 21, 2019

Choose a reason for hiding this comment

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

Weren't you saying that you don't want business logic in your widgets?
Knowing when to fetch is business logic.

Similarly your previous code had a bug, where if you modified the postId from the comment page itself — it wouldn't load the new comments.
With this code, that bug never happens.

The throw is unnecessary, but I kept your busy thing so I felt it was natural.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes I did. When I say widget I mean the file that you describe as meta data to the UI and to which I refer to as the UI code. All my business logic is in my Model for the widget, not the widget itself. The bug wasn't there because I was using get_it over provider, it was just a bug based on the tutorial code. My tutorial is always the minimum amount of code to get my idea across and to teach the concept at a higher level.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not saying that the bug was there because of get_it :)
Just that this architecture fixed it without even trying to.

initialRoute: '/login',
onGenerateRoute: Router.generateRoute,
),
return MultiProvider(
Copy link
Owner

Choose a reason for hiding this comment

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

First of all thank you so much for taking the time to write out this example. I really appreciate it. It gives me insights and some other ideas on how I might use it. But this implementation is definitely not for me.

This is exactly what I was talking about with the verbosity of using Provider only. It can definitely be done, I did it this way and as I said over on reddit. Things got way too messy. This is what I consider to be messy (in terms of too much code for no additional gain over a much cleaner approach).

The class went from 28 lines to 89. Remember that I want to teach the ideas of setting up architectures, if someone wants to implement this afterwards I think they can. Showing a beginner all of this in my tutorials would probably scare some of them away and make them look for a better approach, which is what happened to me. I personally will not be using this method of injection because of how long and verbose it is. I like code that's easier to read and manage, it makes my job more fun and easier to share with other developers. I know in the Functional community and the developers moving away from OOP this is a laughable concept, and in that mindset I do agree. But at the end of the day the dev is the one spending hours in a code base, something as verbose as this (even though it has less dependencies) is not something I want to look at for 300+ hours and explain to new developers coming in or when handing over code bases.

Copy link
Author

@rrousselGit rrousselGit May 21, 2019

Choose a reason for hiding this comment

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

Bear in mind that I deleted both locator.dart and base_view.dart. I also handled the dispose of everything, which you forgot.
And once the todos are applies, each proxyprovider will loose 3 lines of code.

So overall the code size if the same.

Also, you can extract the MultiProvider into the original locator file if you want to.

Copy link
Author

Choose a reason for hiding this comment

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

Same goes for all the models that I wrapped into a custom getter to make the field immutable.
Optional. It just prevents silly mistakes where others think they can modify it.

Copy link
Owner

Choose a reason for hiding this comment

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

I get all of that, and I can see that it can be done with Provider only. I just don't find it as neat as

locator.register(MyObject()); 

That's one line compared to

ProxyProvider<DependentService, MyObject>(
          builder: (_, dependentService, previous) =>
              (previous ?? MyObject())..dependentService= dependentService,
          dispose: (_, obj) => obj.dispose(),
        ),

Although Kudos for the disposing, I did not show that in this tutorial and it can be removed ofcourse to reduce. But again, reading the top line is easier than reading the bottom line. I also still don't see a clear advantage over using provider only over get_it. They're both doing the same thing, one makes your code easier to understand (in my opinion) and the other doesn't.

Copy link
Author

Choose a reason for hiding this comment

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

  • auto updates of MyObject dependencies
  • auto updates of anything that depends on MyObject
  • overridability (a part of the screen can use a different MyObject if needed)
  • control over which parts can access MyObject and which cannot.
  • clear hooks on where we need to instantiate the object, and where we should dispose of it.

But I'll think of a way to improve the readability of such a mutable ProxyProvider.

Maybe something like that:

ProxyProvider<Dependency, Output>(
  initialBuilder: (_) => Output(),
  builder: (_, dependency, output) => output..dependency = dependency;
  dispose: (_, output) => output.dispose(),
)

Is that better?

Choose a reason for hiding this comment

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

It is a lot better, really like the idea. I'm looking forward the merge of the ProxyProvider to start using provider. Thanks you both for the discussion, as it really let me learn a lot of things. This example was really good and complete.

In relation with Provider and the changes you did in the other pull request, some providers names feel weird, such as ChangeNotifierProxyProvider2. Another thing that scares at first sight is the large number of different providers available. I hope the differences between them will be well documented.

Copy link
Owner

Choose a reason for hiding this comment

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

I was hoping we can all learn from the discussion. I'm happy to hear you gained some insight from it.

Copy link
Author

Choose a reason for hiding this comment

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

@yaymalaga

ChangeNotifierProxyProvider vs ProxyProvider has the same relationship than ChangeNotifierProvider vs Provider.

The only difference between ChangeNotifierProxyProvider and ChangeNotifierProvider is that the latter has no dependency on other providers, while the former does.

And then the 2 at the end of ChangeNotifierProxyProvider2 is the number of dependencies this proxy-provider has.
That 2 is sadly necessary because there are no variadic/optional type-parameters in Dart.

Is that clear enough?
That's a very important point you brought up! 😄

Copy link
Author

Choose a reason for hiding this comment

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

@FilledStacks yeah, thanks again for the conversation!
It was insightful in many ways!

Copy link

@yaymalaga yaymalaga May 21, 2019

Choose a reason for hiding this comment

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

And then the 2 at the end of ChangeNotifierProxyProvider2 is the number of dependencies this proxy-provider has.
That 2 is sadly necessary because there are no variadic/optional type-parameters in Dart.

Totally clear, then it is not a big deal. Thanks!

Copy link
Owner

@FilledStacks FilledStacks left a comment

Choose a reason for hiding this comment

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

Lots of great changes, but a lot of changes that would make the code more difficult to teach. The main idea with the tutorial was to explicitly cover certain scenarios, but also get the ideas across into how to structure code specifically for your use cases. Teaching architecture is something not being done in the community and wells structured code will make for better OOP developers.

I really appreciate these changes and you taking the time to make them to demonstrate your point. This community is so awesome, I can always count on great feedback like this. Unfortuntely the amount of additional code, just to get get_it out of there is a no-go for me. The things I will be taking is:

  • json serializable for the data models
  • the widget annotation for declaration (great package)

Those are the kind of code changes I'd definitely make to my application. Again, much appreciated, I'll make this code available in my tutorials repo as example of using provider only so my viewers can still see it if they want to.

@rrousselGit
Copy link
Author

rrousselGit commented May 21, 2019

There is no more code than before.
The code moved around, and I added bonus checks – but that's it.

I opened another PR (#4) where I removed all the generated files and "extra" stuff, like my comments or similar. The code won't work by itself since I haven't merged the new stuff like ProxyChangeNotifierProvider2 though.

You can see on the git diff that there are 322 lines removed for 324 lines added (and I reverted json_serializable which cheats a bit)

So we can see that the code is pretty much the same.

@rrousselGit
Copy link
Author

rrousselGit commented May 21, 2019

Anyway, the differences are:

  • The data-flow is now forced to be uni-directional.
    By removing the singleton from get_it and using provider only, it becomes impossible for a change in the CommentsModel to have side-effects on User for example.

  • Values are always up to date.
    If for whatever reason, you decide to replace Api with a different instance – then all objects that depend on it will automatically update.

  • It is more optimized
    Only widgets that depend on a value rebuilds when it changes. No less, no more.

And I haven't made the change but:

  • With provider we can prevent LoginView from reading the current user. Similarly, HomeView could be unable to read from CommentsModel. That's not something get_it can do, and this forces a better architecture overall.

@FilledStacks
Copy link
Owner

I agree that those two differences are there and that they might make some things better although in the apps I've built this would not make a difference.

it becomes impossible for a change in the CommentsModel to have side-effects on User for example.

This would only be possible if there's specific code written to do this. I know that you're saying it's possible but it still has to be done explicitly.

If for whatever reason, you decide to replace Api with a different instance

haha, this is true. I've always had a problem with the idea of providing solutions to non-existing problems. This is a very neat feature to have but will rarely take place especially since Flutter apps cannot download new code and replace existing code, while the app is running, with a new instance.

That's not something get_it can do, and this forces a better architecture overall.

Get_it definitely cannot do that but I disagree on "forces a better architecutre". If you said forces a more strict architecture I would agree.

There is no clear advantage to me (maybe it's flying over my head) in using provider only for dependency injection or supplementing it with get_it to handle injections better on in the code outside of the UI (meta data) files. The uni-directional flow of data is definitely more in line with Flutter, and i might be fighting that a bit using get_it, but I don't think I'm fighting it to prevent the directional flow of information, more along the lines jumping over it to make my code more fun to work with and easier to maintain.

@rrousselGit
Copy link
Author

Thanks for your time by the way!
I'm investing in that example because I'm writing an article on "Why provider". So this conversion is very important to me! 😄

This would only be possible if there's specific code written to do this. I know that you're saying it's possible but it still has to be done explicitly.

No that's baked in. In the current changes if CommentsModel tries to modify User, that will throw an exception.

But will rarely take place especially since Flutter apps

Right, for the Api class it's not very useful. But that applies to all objects, and without having to handle anything specifically for it.

There is no clear advantage to me

Of course, you can do everything that you do with provider without it.
The difference is that with provider you can "unplug your brain" 😄

get_it is technically fine, but it requires some discipline/experience. You can't give get_it to a junior expect him to produce good code directly.

provider is a lot stricter, but it's a lot harder to make code that is not reusable/testable and without tons of undesired side-effects.

@FilledStacks
Copy link
Owner

It was absolutely my pleasure, I enjoyed this conversation a lot and can say confidently I know 80% more about provider than I did before this conversation. Which is probably why I used only 10% of it's capabilities.

I agree with a lot of what you are saying and I'm very excited for your article. There's probably a lot to learn and a lot I can change in this architecture setup, or maybe get a completely new one based on gaining more experience with Provider.

Thank you very much for the constructive feedback and criticism of the setup, it's a very valuable discussion to me as well. It's a big step in me trying to improve my knowledge and skillset as well around state management and architectures in Flutter.

@rrousselGit
Copy link
Author

😄

By the way, folks using provider needs some examples. Would it be fine with you if I used this app (with your name mentioned in the readme) as an example?

I'm planning on making the same app using provider + [ChangeNotifer, mobx, BLoC, flutter_bloc]

Also, feel free to close this PR and #4

@FilledStacks
Copy link
Owner

Yes! you can absolutely do that 😁 I would be very happy if you mentioned it. Even if you say "this is not how it should be done" haha 😆

Thanks for the discussion. I hope I don't miss your article when it comes out.

@ashishkj93
Copy link

how to rest api with authentication login and customauthentication

@ashishkj93
Copy link

I have created a splash screen , login screen and homescreen .
how to auto login with restapi

@ashishkj93
Copy link

can you give me answer plesase?

@FilledStacks
Copy link
Owner

😂 @ashishkj93 After initial login set a value in your shared preference to true, check that value on startup and navigate either to login view or a different view. Watch my startup logic video on YouTube. or read mt startup logic snippet on my website for a method using future builder.

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

Successfully merging this pull request may close these issues.

None yet

4 participants