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

Introduce a UiState class for the architecture pages #11437

Open
1 task
alefwd opened this issue Nov 24, 2024 · 8 comments
Open
1 task

Introduce a UiState class for the architecture pages #11437

alefwd opened this issue Nov 24, 2024 · 8 comments
Assignees
Labels
from.page-issue Reported in a reader-filed concern

Comments

@alefwd
Copy link

alefwd commented Nov 24, 2024

Page URL

https://docs.flutter.dev/app-architecture/guide

Page source

https://github.com/flutter/website/blob/main/src/content/app-architecture/guide.md

Describe the problem

Similarly to Android Architecture Components, the UI layer should introduce a well defined UiState class.
The use of a UiState class has many benefits, as it allows a clear declaration of what is part of the UI state, it helps reasoning about it and it helps handling it, without having the code scattered into another class.

Here one example of a generic UiState class I use (with freezed):

@freezed
class AlePageUiState with _$AlePageUiState {
  const factory AlePageUiState({
    @Default([]) List<ItemUiState> items,
    @Default(false) bool loading,
    @Default(false) bool error,
    @Default('') String errorMsg,
  }) = _AlePageUiState;
}

The UI state class doesn't necessarily need to include the loading and error, but keeping them included helps to represent better the formula:

UI = f ( state )

because anything that change the UI has to originate from a change in the state, including the loading state where we usually want to show a loading indicator and the error state where we usually want to show a snackbar.
Having them included in the UiState class helps to reason in terms of a (in)finite state machine:

Gc0EfiUXIAAC3x2

All the material taken from here, where you can find also a code repository, a video and the slides.

Expected fix

Adding a section titled "Defining the UI state" or similar.

Additional context

No response

I would like to fix this problem.

  • I will try and fix this problem on docs.flutter.dev.
@alefwd alefwd added the from.page-issue Reported in a reader-filed concern label Nov 24, 2024
@vgtle
Copy link

vgtle commented Nov 26, 2024

I like the idea, but I would argue against some third-party package like freezed to be included. The example should be build using only the components Flutter comes with. It should be clear how the general concept works and how this can be done in Flutter directly. The focus should not be about how freezed works, what it generates, and what the different annotations refer to.

@miquelbeltran
Copy link
Member

FYI, there is still documentation for the Flutter Architecture guidelines in the works.

A guide with practical implementation details should land soon: #11414 and there will also be a cookbook recipe on the Command pattern.

IMO the presented UiState has some similarities with the Command pattern that the compass_app example uses, as it also contains the running error and completed states.

When implementing the compass_app example, we evaluated refactoring ViewModels to extract the UI state into a separated object, but we couldn't see the advantage against just having those inside the ViewModel/ChangeNotifier. If anyone is interested in it, this was the discussion on the PR: flutter/samples#2398

I'd say it is up to the developer how they decide to organize the data in the ViewModel, if directly in the class or as a separated UI object. This could be added as a side note on the MVVM implementation pages.

@miquelbeltran
Copy link
Member

In fact, this is going to be mentioned in the upcoming docs:

image

So I think that covers this request.

You can see a preview here: https://flutter-docs-prod--pr11414-ew-app-architecture-case-st-5j24ywro.web.app/app-architecture/case-study/ui-layer

@miquelbeltran
Copy link
Member

I would argue against some third-party package like freezed to be included. The example should be build using only the components Flutter comes with. It should be clear how the general concept works and how this can be done in Flutter directly. The focus should not be about how freezed works, what it generates, and what the different annotations refer to.

I know your message was about the UiState, but I think it's probably worth creating a separated ticket to discuss possible alternatives to freezed in the compass_app example and the Flutter Architecture guidelines.

@vgtle
Copy link

vgtle commented Nov 26, 2024

This looks nice and I think it goes into the same direction. I am still not sure if freezed should be used here by just saying that it ensures immutability. This sounds like, there is no other way in Flutter directly to do that. Is there a way to support you?

@vgtle
Copy link

vgtle commented Nov 26, 2024

I would argue against some third-party package like freezed to be included. The example should be build using only the components Flutter comes with. It should be clear how the general concept works and how this can be done in Flutter directly. The focus should not be about how freezed works, what it generates, and what the different annotations refer to.

I know your message was about the UiState, but I think it's probably worth creating a separated ticket to discuss possible alternatives to freezed in the compass_app example and the Flutter Architecture guidelines.

I will open a ticket, thank you 😄

@ericwindmill
Copy link
Contributor

I feel pretty strongly that we shouldn't add more complexity to the base architecture guide. (But I'm always open to being wrong.)

But, as Miguel said, we did discuss this at one point, and I can imagine writing a Design Pattern recipe about this, as those are kind of like "extensions" for the base documentation.

@alefwd
Copy link
Author

alefwd commented Nov 26, 2024

@miquelbeltran thanks for pointing out to the works in progress, definitely the mention covers this request, and I still have hope somewhere you could add a code snippet (side note: freezed is one between so many different ways).
I understand the choice can be left to the developers and be totally optional, I'm fine with that.

When implementing the compass_app example, we evaluated refactoring ViewModels to extract the UI state into a separated object, but we couldn't see the advantage against just having those inside the ViewModel/ChangeNotifier.

by my experience in all these years, having a class where all the fields which contribute to the UI state are well specified helps to limit bugs, while defining them inside a ChangeNotifier can mix them with other fields which don't

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from.page-issue Reported in a reader-filed concern
Projects
None yet
Development

No branches or pull requests

4 participants