🦺 Code Style & Best Practices

We use lints wherever feasible to have a well documented and automatically enforced set of style guides and rules (see below). However, there are a few cases where over time certain best practices and patterns have shown itself as useful and will additionally be enforced by PR reviewers throughout the entire code base. While we intend to make all of them automatic lints at some point, at this point they are show and explained here.

General Style with default lints

We generally follow the default cargo fmt-formatting & clippy rules for native, and the default flutter and dart lints on the App. You can run cargo make fixup to have the style automatically – especially when the CI complained. Beware, it doesn't fix all of them though.

All lints must be fixed or addressed (some might be ignored for that line or file if that is okay in that context) before a PR can be merged. The github action will block a merge.

Riverpod Best Practices

We use riverpod as our main state management system. Over the years, we have come to learn a few pitfalls and best practices to avoid shooting ourselves into the foot.

Smaller providers are better, chaining is encouraged

Ideally a provider does exactly one thing and one thing only. The more things are done in one provider the longer it takes it to evaluate and the more complicated it becomes to maintain. Instead, having smaller stupid providers that are chained by them using ref.watch internally to wait for other provider is highly encouraged.

You can also simplify the chain busing .select((x) => x.internalThing to make sure the provider doesn't trigger an evaluation if the underlying thing hasn't changed. See the docs for select to learn more.

When to use ref.read and when ref.watch

ref.read only reads the value at the given time. If it is being updated, nothing happens. While ref.watch will also observe the value and issue a re-calculation when the internal value changes. Thus, ref.read shouldn't be used in any build function, you most certainly want to use ref.watch there to ensure the UI always updates accordingly. ref.read can (and should) be used for single-instance usage, for example on events or within actions that are being executed once.

Data-based Calculations belong providers, UI stuff in the Widgets

Riverpod is great at caching, cascading and chaining state management. But it should always be based on the actual data model underneath. Not for anything of the UI itself, like the Media-Size, Theme or similar - that's what the Widgets are great at and the build-Function is made for. So, if you ever find yourself passing BuildContext to a provider, stop and think: that's not how it supposed to be, this belongs into a Widget/Consumer.

Keeping providers pure and in cascade chain has several advantages, like caching and stopping the chain as soon as thing haven't actually updated any further. BuildContext and any .of(context)-Type-thing has no reason to be there, these are purely about the way these things are presented and that can change for many reasons (e.g. switching the language, resizing the window, rotating the screen ...), which have nothing to do with the underlying data and should not that be causing it to trigger again. As data computations might be heavy, any unnecessary recalculation should be avoided.

Instead the build functions of widgets are meant to be re-evaluated many times a second and are perfectly equipped to deal exactly with that. This also avoid the problem of mixing the data-model in the providers with the visualisation model of the widget. The latter is the job of the widget and the widget only.

Avoid setState for provider data

Riverpod providers automatically update constantly, in particular because content can always come down from the server through the SDK updating the internal state of objects. When you use setState or calculate something in initState you are going to miss these updates. While the power of riverpod's state management lies within exactly those cascading updates. So avoid doing that to not have the UI run out of sync with the underlying data.

There are, however, some pretty rare circumstances in which you need to put the data into local state. Just be sure to use listenManual in initState and deal with all possible cases that need to be updated within that.

Don't requireValue

There is always cases where that is not true and due to its throwing nature this then causes entire screens to break. Do not use it, instead fall to a valueOrNull? ?? pattern.

Don't

final someData = ref.watch(someAsyncProvider).requireValue;

Do

// accept it to be a null
final someData = ref.watch(someAsyncProvider).valueOrNull;

// or with a default value:
final someList = ref.watch(someAsyncProvider).valueOrNull ?? [];

Flutter State and Widgets

don't isDesktop use actual MediaQueries

Acter is built in a responsive manner. Meaning that upon resizing the screen, we try to make the best use of the space available as possible. Checking for whether the provided platform is a desktop platform is largely misleading as a tablet in landscape setting might have the same space available. Instead of checking for isDesktop, look at MediaQuery.of(context).size or - even better - use our context.isLargeScreen helper that already checks for our common breakpoints and make things smooth across several screens.

Don't

child : isDesktop ? DesktopUI() : mobileUI(),

Do

child: context.isLargeScreen ? largeScreenUI() : mobileUI(),

Pure URLs routing only

GoRouter allows for you to pass basically anything via extra, including entire objects. Similarly, we have seen several patterns where a ref.read(someProvider.notifier).state = XYZ is being called right before routing tot the screen to set some desired internal state. Don't do that.

We assume that internal routing URLs are pure. Meaning that that we can express everything we want to happen through the given path and query parameters and any internal state management happens on the routing controller or when the Widget is initialized. As the caller, other than constructing the route, I am not expected to have any further knowledge of the internal system or its structures nor to set anything like it.

Reason: It is hard to keep track of all the spaces a route is being called from and that when updating the internal state management, all callers adhere to that new way of doing it. Similarly, it is hard for a reviewer to notice that any of those assumptions might be broken by a new caller. To avoid these scenarios, and the the following misbehaving app and bad UX, we have agreed that no such assumptions may exist and instead that all the caller "API" is and must be expressed through pure URLs only.

Bonus point: it makes it possible to share these URLs throughout the app and be sure that they work as intended even if called from anywhere else.

Colors & Fonts come from the Theme

Do not hard code any colors or types fonts. We are basing all our coloring system on the relative and dynamic theme.

Don't

color: Colors.black, 

Do

color: Theme.of(context).surfaceBackgroundColor

Guard for null instead of null checks (!)

It is a common suggested fix in most IDE's to just slap a ! behind a potential null value, if a full value is expected. Avoid that at any cost. Most IDE's and lints already pick up correctly that you already did an if (value != null) check beforehand, do that instead. Or if a default value is sufficient, supply that instead via ?? defaultValue.

Reason: _TypeError Element.widget Null check operator used on a null value are a very commonplace error we see all the time. You might assumed this wasn't ever going to be null when writing that code, but the number these errors come clearly shows we are very often wrong. Or also, that assumption might have been broken at a later point and it now break. In the base case scenario it means the screen doesn't render for the user but even then, the stack trace we get gives us essentially nothing to track down the issue. In the worst case scenario, this breaks routing or crashes the entire app.

Native rust rules

Don't unwrap in production code

While .unwrap() is fine in test code, we do not allow it in production code. It is always recommended that if a function can internally fail, that failure should bubble up by using ? instead.

Reason: .unwrap are hard failure. They crash the app, leading to a terrible user experience. Even on an I/O error or other problems that are hard failures we can't continue to operate under, it is better to tell the user about it and have them actively restart the app.

Use ? and bail! to verify user input

.unwrap, .expect and panic are equally discouraged for any case of supposedly known input values. Even if they are typed, if they can lead to a Result in unwrapping you should use ? and bail! , especially when that input can be directly manipulated by the user (as in, the user of the API).

On using .expect instead
IF you are certain this won't ever fail and thus don't want to create that overhead, you may use .expect( in those cases with a unique message describing the assumption why we'll never see that being a problem. The reason it must be unique is so that when we see that string, we can find it in the code base quickly, understand what assumption has been broken and who is to blame and should fix it (probably you for putting a .expect in there. So don't do it!)

.context(..) are for Options only

We use anyhow for simple string-based errors. However, if that is being used on an Result it covers up the internal error and gives a potentially wrong error message. .context should only be used on Option to map a None to a predictable error. If what we have is a Result, then ? or map_err(..) should be used to bubble up the actual error and stack

Don't

fn someFn(String input) -> Result<()> {
   let value = OwnedId::parse(input).context("Didn't parse");
   ...
}

Do

fn someFn(String input) -> Result<()> {
   let value = OwnedId::parse(input)?;
   ...
}

Further Tips & Tricks

Create lang to get around the build_context_await_points problem

We have several areas in the flutter code, where we use await around actions and want to show the users translated strings afterwards. The flutter lint checker (righteously) complains about that and recommends to do a if (context.mounted) before. This becomes very verbose and annoying quickly. Instead, we often only need the BuildContext because we do L10n.of(context).someLangString. So instead, what we recommend is doing a final lang = L10n.of(context); at the top of the function and just use lang.someLangString instead for those circumstances. It is less verbose and an accepted way to deal with that lint problem.