Create compass-app first feature

[miquelbeltran]

As part of the work for the compass-app / architecture examples

This PR is considerably large, so apologies for that, but as it contains the first feature there is a lot of set up work involved.
Could be easier to review by opening the project on the IDE.

Merge to compass-app not main

cc. @ericwindmill

Details

Folder structure

The project follows this folder structure:

• lib/config/: Put here any configuration files.
• lib/config/dependencies.dart: Configures the dependency tree (i.e. Provider)
• lib/data/models/: Data classes
• lib/data/repositories/: Data repositories
• lib/data/services/: Data services (e.g. network API client)
• lib/routing: Everything related to navigation (could be moved to common)
• lib/ui/core/themes: several theming classes are here: colors, text styles and the app theme.
• lib/ui/core/ui: widget components to use across the app
• lib/ui/<feature>/view_models: ViewModels for the feature.
• lib/ui/<feature>/widgets: Widgets for the feature.

Unit tests also follow the same structure.

State Management

Most importantly, the project uses MVVM approach using ChangeNotifier with the help of Provider.

This could be implemented without Provider or using any other way to inject the VM into the UI classes.

Architecture approach

• Data follows a unidirectional flow from Repository -> Usecase -> ViewModel -> Widgets -> User.
• The provided data Repository is using local data from the assets folder, an abstract class is provided to hide this implementation detail to the Usecase, and also to allow multiple implementations in the future.

Screenshots

Extra notes:

• Moved the app code to the app folder. We need to create a server project eventually.

TODO:

• Integrate a logging framework instead of using print().
• Do proper error handling.
• Improve image loading and caching.
• Complete tests with edge-cases and errors.
• Better Desktop UI.

Pre-launch Checklist

• I read the Flutter Style Guide recently, and have followed its advice.
• I signed the CLA.
• I read the Contributors Guide.
• I updated/added relevant documentation (doc comments with ///).
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-devrel channel on Discord.

[miquelbeltran]

This was necessary to run on macOS

[craiglabenz]

Looking great! Just a few notes, but this is clean.

[craiglabenz]

Is there an app architecture guarantee that this list will only be accessed when strictly necessary? I ask because I believe each access run the fill innards, reinitializing everything, instead of reusing anything.

[miquelbeltran]

At the moment, this dependency tree is created on runApp() and indeed this function shouldn't be called elsewhere, so it is only called once to setup the top level Providers.

As the dependency tree becomes more complex (and we add config options to it) we will have to convert this getter into a function, and we should be able to cache all those dependencies that don't need to be rebuilt.

Another consideration could be making this a private method in the main.dart file to ensure it is not called elsewhere.

I think we will come with a better solution in the future when we tackle the Dependency Injection section in the Architecture docs.

[craiglabenz]

Should there be isOk and isError boolean getters, as well? Either way, usage docstring to suggest the right way to interrogate a yet-ambiguous Result instance would be nice.

[craiglabenz]

I see later on that, because this is a sealed class, pattern matching works for this. That's great! A quick docstring suggesting that would still be useful, IMO :)

[miquelbeltran]

Good point! I will add that.

[craiglabenz]

I don't think it's strictly required to use code generation for data classes, but we should at least have a clear articulation of why we are not. For example, this Destination class lacking value equality could cause problems later on.

I am personally quite a fan of pkg:freezed, though that is just one of multiple possible choices.

[miquelbeltran]

Agreed! Freezed could be a good option for this project, and we probably want to integrate it later on, as we add more features.

[ericwindmill]

This would be a good opportunity to 'be opinionated'. I agree that we probably want to add some sort of equality checking at the least and potentially code generation.

@miquelbeltran this isn't something that we need to add in this PR necessarily, but good to consider for later.

[craiglabenz]

Does pkg:cached_network_image deliver everything needed?

[miquelbeltran]

I think so, except if the team has a different suggestion, I'd go with that as well.

[loic-sharma]

FYI, the following files are generated by the Flutter tool and shouldn't be checked-in:

1. windows/flutter/generated_plugin_registrant.cc
2. windows/flutter/generated_plugin_registrant.h
3. windows/flutter/generated_plugins.cmake

[johnpryan]

Why is the code in the compass_app/app directory and not compass_app/?

[reidbaker]

Why is the code in the compass_app/app directory and not compass_app/?

I believe because there will be a server component at some point.

[miquelbeltran]

That's correct, the example will run with a dart server app as well.

Thanks for all the feedback everyone! I'm currently OOO without my laptop but will get to it soon.

Glad that we keep the Result class. If anyone has ideas on how to improve it lmk as well.

[miquelbeltran]

I took the liberty of re-writing this sample with my feedback applied, here's a zip containing the lib directory with the changes I would like to see: lib.zip

Thanks for taking the time to prepare a code example @johnpryan, it was very useful to visualize your proposal. I took a look, and also reply to your points before.

Remove use cases

While I like them, I agree that for the sake of simplicity in the example we should avoid them until strictly necessary, so I will refactor the code to remove it.

Don't use a global ViewModel:

I agree with the general idea of now making the ViewModel a long-lived top-level dependency.

What I am not sure, is if we want the widgets to create and manage the lifecycle of its ViewModel` or if ViewModels should be created outside them and then injected somehow.

The FWE MVVM part doesn't really address this: https://docs.flutter.dev/get-started/fwe/state-management#using-mvvm-for-your-applications-architecture For example, looking at Majid's Flutter Engineering book, there the ViewModels are created outside the Widget and passed as constructor param. So I feel everyone has a different opinion on this :)

I personally would rather wrap the ResultsScreen with the ChangeNotifierProvider, like this:

  return ChangeNotifierProvider(
    create: (context) => ResultsViewModel(
      destinationRepository: context.read(),
    ),
    child: const ResultsScreen(),
  );

And this code would be in the GoRoute builder for /results.

Testing is of course a bit more difficult than passing it by parameter, since it requires you to still wrap the Widget with the ChangeNotifierProvider, so it has its disadvantages.

Use ListenableBuilder:

Related to the previous point.

Since we are using Provider, this allows us to use ChangeNotifierProvider + Consumer directly in the ResultsViewModel.

While I think there is value in understanding how to manage the lifecycle of a ViewModel within a widget and use ListenableBuilder, I would think many developers would not understand why aren't we just using those two tools that Provider has, since we already incorporate the package.

I think there will be value in presenting the different alternatives nevertheless in the documentation pages we will prepare.

Folder naming conventions I prefer using the terms models, view_models, and widgets, rather than business, data, and presentation, but I'd love to know what others think.

I am used to them, to me "data layer" and "presentation layer" seem very standard when talking about general software architecture, so it would be consistent with the vocabulary we use when documenting the architecture design.

It will be unlikely we have a middle "business layer" if we don't need use cases or similar.

But I have removed the presentation folder and instead created view_models and widgets for each feature, as it makes more sense since we don't have usecases either.

Still prefer to keep all "data" parts, e.g. repositories, models and services, in the data folder.

Use relative imports:

Perfectly fine for me, if we decide to adopt it for this, we should set up https://dart.dev/tools/linter-rules/prefer_relative_imports (and maybe even should become a default for flutter_lints?)

---

Edit: I have pushed now some changes based on the feedback

[miquelbeltran]

FYI, the following files are generated by the Flutter tool and shouldn't be checked-in:

1. `windows/flutter/generated_plugin_registrant.cc`

2. `windows/flutter/generated_plugin_registrant.h`

3. `windows/flutter/generated_plugins.cmake`

The .gitignore doesn't include them, neither any of the other GeneratedPluginRegistrant.*. Not sure if that's intentional or not.

[miquelbeltran]

@TytaniumDev got your feedback and implemented a ThemeExtension for the TagChip widget. I may do the same for the ResultCart title later on.

Also implemented a basic light/dark theme configuration.

[ericwindmill]

Don't use a global ViewModel:

I agree with the general idea of now making the ViewModel a long-lived top-level dependency.

What I am not sure, is if we want the widgets to create and manage the lifecycle of its ViewModel` or if ViewModels should be created outside them and then injected somehow.

Use ListenableBuilder:

Related to the previous point.

Since we are using Provider, this allows us to use ChangeNotifierProvider + Consumer directly in the ResultsViewModel.

While I think there is value in understanding how to manage the lifecycle of a ViewModel within a widget and use ListenableBuilder, I would think many developers would not understand why aren't we just using those two tools that Provider has, since we already incorporate the package.

I agree that we shouldn't be using a global ViewModel, and it should be created in the widget itself, like @johnpryan's example. But I also agree with @miquelbeltran that if we're using provider at all, readers would wonder why we're making extra work for ourselves by not using all of it's features.

The more I think about this project, the more I think we should favor using packages less, and keeping it vanilla as possible. Big companies with complex apps (and solo devs, for that matter) are going to use this as a starting point, and add packages and remove other packages to meet their specific needs. Starting more vanilla seems like it will better serve our target audience.

I don't have a strong opinion on which approach we go with, but I'm curious what everyone thinks here. @miquelbeltran @johnpryan @reidbaker

Folder naming conventions I prefer using the terms models, view_models, and widgets, rather than business, data, and presentation, but I'd love to know what others think.

I am used to them, to me "data layer" and "presentation layer" seem very standard when talking about general software architecture, so it would be consistent with the vocabulary we use when documenting the architecture design.

Is there a dart standard naming convention? Do the Dart/Flutter teams have a standard? For example, do they always have a common folder for shared resources? @reidbaker @johnpryan

I'm not aware of any standards, but if there , we should go with those. Obviously this is trivial to change in the future, so we don't need to fret about it now.

[reidbaker]

"What I am not sure, is if we want the widgets to create and manage the lifecycle of its ViewModel` or if ViewModels should be created outside them and then injected somehow."

This could be a place where my mental definition does not match the use in this pr but to me models are dart object representations of the underlying truth of some data. view_models are specific objects used by a widget (or group of widgets) to represent something on the screen.

By my definition view_models should be created outside of the hosted widget as a stream (the concept not necessarily the Stream class) then when a new view_model is created the widget rebuilds. This lets you test your view_model production logic independently of your presentation logic. It also means you can write widget tests for view_model objects that your view_model production logic can't generate. Like for example if you had a user profile without a name you could ensure the widget didnt crash and maybe showed empty if that data was missing AND you could write a test in your view_model production code that ensured that if name was missing you used a translated string for "missing name please contact support".

"Folder naming conventions I prefer using the terms models, view_models, and widgets, rather than business, data, and presentation, but I'd love to know what others think."
I have a pretty strong objection to using a flutter class name in the folder structure. It leads to misunderstandings about what the folder is supposed to contain. FWIW my preference is a wild departure from what we currently use.

• view_model = dart object containing the data needed for a set of widgets to display some information
• model = dart object representing the truth about local or remote state
• repository = class responsible for emitting model objects and handling retry logic and error emitted from services
• service = class responsible for api calls (local and remote) and converting those calls into dart methods and objects. plugin_service, the same as a service but requires platform implementations
• view = flutter widgets that take a view model and display their contents and attach their callbacks
• command = flutter callbacks in a view_model to attach to buttons (or other views) when they are interacted with
• bloc aka business logic = dart code that takes streams of model objects from repositories and converts them into view models
• component (possibly usecase here) = the combination of bloc, view_model and view that is an independent logical unit of functionality that can be placed anywhere in the app.
• screen = flutter route that can be used to navigate, screens do not have business logic and are simple widgets used to format components

"It will be unlikely we have a middle "business layer" if we don't need use cases or similar."
I think we need to show the concept of displaying something different than the base model object. This is part of the core logic large apps have to deal with.

"The more I think about this project, the more I think we should favor using packages less, and keeping it vanilla as possible. Big companies with complex apps (and solo devs, for that matter) are going to use this as a starting point, and add packages and remove other packages to meet their specific needs. Starting more vanilla seems like it will better serve our target audience."

I dont think people will use this app as a starting point. They will use the principles or concepts here and adapt them to their existing project. I think this apps needs to be less 'starting point' and more of an example for how they can accomplish different aspects of a 'real" app.

"Is there a dart standard naming convention? Do the Dart/Flutter teams have a standard? For example, do they always have a common folder for shared resources?"

Every app I have seen has a "common" so I didnt argue for its exclusion but it really is the worst folder/package name because it gives no indication what should be included or excluded from the folder/package. It grows until there is a circular dependency then code is broken out with a more specific designation. My view is that this is our attempt at articulating a standard.

[TytaniumDev]

One thing I'd like to add to the discussion as well is the MVVM architecture's performance. I imagine lots of people will assume this is the most performant state management strategy because Google is backing it.

An issue I've seen with how my previous teams have used MVVM is that the viewmodels end up becoming large, and the amount of individual variables that we want to render in a view from that viewmodel grows to the point where our view is rebuilding quite often when it doesn't need to.
While this can be fixed by enforcing smaller views/viewmodels as a team, it's also an interesting problem that I feel could be improved by using other existing Flutter features, perhaps something like InheritedModel where any individual Widget in a View could listen to updates for just the field(s) on the ViewModel that they're interested in?

I don't think this needs to be addressed in this PR but I think it's something good to start thinking about.

[TytaniumDev]

Oh this would be an excellent spot to demonstrate good theming defaults best practices! It's common in many of the Flutter material widgets, like here:

Basically, there's an order of priority of where themed elements of a widget come from. They're used in this order:

1. Arguments passed in through the widget's constructor
2. The widget-specific theme, grabbed from the context (in our case the theme extension defined below)
3. A default baked in to the widget itself

If you added a chipColor and onChipColor to the constructor of this widget, we'd be able to follow that pattern.

This pattern is really great as it gives devs flexibility to theme one-off widgets easily, while still having an app-wide theme to fall back on.

[miquelbeltran]

Good idea! I have copied your comment into our internal doc to comment on this when we talk about widget design

[johnpryan]

@miquelbeltran thanks for addressing my feedback! Here are my thoughts on the dependency-injection patterns we should be recommending:

I agree with the general idea of now making the ViewModel a long-lived top-level dependency.

What I am not sure, is if we want the widgets to create and manage the lifecycle of its ViewModel` or if ViewModels should be created outside them and then injected somehow.

I don't mind adding the ViewModel object to the ResultsViewModel constructor. This is clearer and makes testing easier, since you can swap it out in a unit test.

The problem with making it a global is when you have two or more views of the same kind that you will need to show at the same time. For example, an app might have a list view that shows all records, or a subset of records based on a query. If they share the same view-model, then its state will be showing whichever view accessed it last. Instead, the thing you want to share between views is probably model.

I personally would rather wrap the ResultsScreen with the ChangeNotifierProvider

I prefer to declare the view-model as a required dependency in the constructor, since it's more clear to the reader that a dependency is being provided to the view.

I personally would rather wrap the ResultsScreen with the ChangeNotifierProvider, like this:

  return ChangeNotifierProvider(
  create: (context) => ResultsViewModel(
    destinationRepository: context.read(),
  ),
  child: const ResultsScreen(),
);

And this code would be in the GoRoute builder for /results.
Testing is of course a bit more difficult than passing it by parameter, since it requires you to still wrap the Widget with the ChangeNotifierProvider, so it has its disadvantages

This gets the job done. It's easy enough to wrap your test code in a Provider, so no concerns there, I do wonder if it's a good recommendation to make from an architecture standpoint though. Consider the case where I want to call Navigator.push() to display a new ResultsScreen. I would need to remember to wrap it in a Provider that provided a new View-Model to avoid the problem I mention above. I worry this could lead to some footguns in a large codebase, and making it more explicit is probably a safer bet.

[johnpryan]

@ericwindmill

The more I think about this project, the more I think we should favor using packages less, and keeping it vanilla as possible

I do feel like it would be more valuable to teach the "vanilla" option, but offer other options (like using ChangeNotifierProvider) that are available to explore.

Is there a dart standard naming convention? Do the Dart/Flutter teams have a standard?

I don't know what exactly to recommend for folder structure. Here's one option:

lib/
  main.dart
  <feature_name>/
    models/ # shared types used throughout the app
    repositories/ # the objects that cache data
    services/ # The objects that fetch and parse remote data
    ui/ # Your beautiful Flutter widgets, organize this folder however you prefer

[johnpryan]

@reidbaker Thanks for writing up those definitions, I like the distinction between "model", "repository", and "service" especially. I don't know if "bloc" means the same thing to to the majority of Flutter developers, but I'd love to learn more about how that works. I would distinguish between "screen" and "route" also, since to me, a "screen" is a useful name for a widget that displays a Scaffold inside, which let's you return a "screen" widget for each route you define in your routing table in a mobile app scenario.

Every app I have seen has a "common" so I didnt argue for its exclusion but it really is the worst folder/package name because it gives no indication what should be included or excluded from the folder/package.

This folder looks OK to me, it's declaring some things that are common to the entire app, but I understand the concern about keeping it tidy.

[reidbaker]

@ericwindmill

The more I think about this project, the more I think we should favor using packages less, and keeping it vanilla as possible

I do feel like it would be more valuable to teach the "vanilla" option, but offer other options (like using ChangeNotifierProvider) that are available to explore.

Is there a dart standard naming convention? Do the Dart/Flutter teams have a standard?

I don't know what exactly to recommend for folder structure. Here's one option:

lib/
  main.dart
  <feature_name>/
    models/ # shared types used throughout the app
    repositories/ # the objects that cache data
    services/ # The objects that fetch and parse remote data
    ui/ # Your beautiful Flutter widgets, organize this folder however you prefer

The issue is that your model data is not tightly coupled with your features most of the time. Even in architectures where you have an app facing api and the design intent is closely map to what apps need (uncommon but sometimes exists) you wind up with feature drift where the apis do not exactly align. To get the type of alignment that would encourage ui and data to live together you basically have to get to a level of sync where you are doing server side rendering.

[reidbaker]

@reidbaker Thanks for writing up those definitions, I like the distinction between "model", "repository", and "service" especially. I don't know if "bloc" means the same thing to to the majority of Flutter developers, but I'd love to learn more about how that works. I would distinguish between "screen" and "route" also, since to me, a "screen" is a useful name for a widget that displays a Scaffold inside, which let's you return a "screen" widget for each route you define in your routing table in a mobile app scenario.

Every app I have seen has a "common" so I didnt argue for its exclusion but it really is the worst folder/package name because it gives no indication what should be included or excluded from the folder/package.

This folder looks OK to me, it's declaring some things that are common to the entire app, but I understand the concern about keeping it tidy.

I will say that my understanding screen and route are weak. It is likely your distinction is the correct one and I am conflating them because I mentally have them to be conflated.

[miquelbeltran]

The problem with making it a global is when you have two or more views of the same kind that you will need to show at the same time. For example, an app might have a list view that shows all records, or a subset of records based on a query. If they share the same view-model, then its state will be showing whichever view accessed it last. Instead, the thing you want to share between views is probably model.

Consider the case where I want to call Navigator.push() to display a new ResultsScreen. I would need to remember to wrap it in a Provider that provided a new View-Model to avoid the problem I mention above. I worry this could lead to some footguns in a large codebase, and making it more explicit is probably a safer bet.

Thanks @johnpryan those are good points.

Later today, I will do one final refactor on this PR injecting the ViewModel in the constructor instead of using Provider, and depending on how it looks probably merge this PR. (there will be plenty of opportunities to refactor this later on if we think we did the wrong decision)

Even in architectures where you have an app facing api and the design intent is closely map to what apps need (uncommon but sometimes exists) you wind up with feature drift where the apis do not exactly align.

This has been my personal experience as well.

@ericwindmill I have copied many comments from the thread into the "Appendix and Research" section of the document.

[miquelbeltran]

Summary of the latest changes:

• The ViewModel is created outside the screen widget and passed as constructor param:

        final viewModel = ResultsViewModel(
          destinationRepository: context.read(),
        );
        return ResultsScreen(viewModel: viewModel);

• As recommended, I have set up relative imports and the corresponding linter rule to enforce it.
• cached_network_image is now setup as well.

Like said before, we can always adjust the patterns later. I will merge the PR and will continue with the rest of the app.