linter

linter copied to clipboard

The documentation in use_build_context_synchronously only provides a minimal 'bad' example (with not much context either).

Proposal

The docs should provide better 'bad' examples for common pitfalls. For example, changing:

BAD:
void onButtonTapped(BuildContext context) async {
  await Future.delayed(const Duration(seconds: 1));
  Navigator.of(context).pop();
}

into one with a StatelessWidget to further drive the point home.

BAD:
class MyStatelessWidget extends StatelessWidget {
 @override
 Widget build(BuildContext context){
  return ElevatedButton(
   onPressed: () async {
     await Future.delayed(const Duration(seconds: 1));
     Navigator.of(context).pop();
   },
   child: const Text('button'),
  );
 }
}

Also, we should add other examples where a widget delegates functionality with an injected BuildContext parameter. For example, consider this bad widget:

BAD:
class MyRetryWidget extends StatelessWidget {
  final Widget? child;
  final bool Function(BuildContext, String)? shouldRetry;
  final Function(BuildContext, String)? onRetryClicked;
  const MyRetryWidget({Key? key, this.child, this.shouldRetry, this.onRetryClicked});

  @override
  Widget build(BuildContext context) {
    return BlocConsumer<MyBloc, MyState>(
      builder: (context, state) {
        final ThemeData currentTheme = Theme.of(context);
        final isDoingWork =
            state.stateType == MyStateType.doing_work;
        return ModalProgressHUD(
          inAsyncCall: isDoingWork,
          color: Colors.black87,
          progressIndicator: CircularProgressIndicator(
            valueColor:
                AlwaysStoppedAnimation<Color>(currentTheme.primaryColor),
          ),
          child: child!,
        );
      },
      listener: (context, state) {
        switch (state.stateType) {
          case MyStateType.work_error:
            final bool shouldDisplayRetry =
                   shouldRetry?.call(context, state.errorCode) ?? 
                   _shouldRetry(errorCode);
            final retry = await showDialog<bool>(
              barrierDismissible: false,
              context: context,
              builder: (context) {
                return PlatformAlertDialog(
                    title: errorTitle,
                    content: errorWidget,
                    actions: <Widget>[
                      PlatformDialogAction(
                          child: PlatformText(shouldDisplayRetry ? "No" : "OK"),
                          onPressed: () {
                            Navigator.of(context).pop<bool>(false);
                          }),
                      if (shouldDisplayRetry)
                        PlatformDialogAction(
                            child: PlatformText("Retry"),
                            onPressed: () {
                              Navigator.of(context).pop<bool>(true);
                            })
                    ],
                );
            });
            if (retry ?? false) {
              onRetryClicked?.call(context, state.errorCode);
            }
            break;
          case MyStateType.success:
            Navigator.pop<bool>(context, true);
            break;
          default:
            break;
        }
      },
    );
  }
}

Note the BuildContext dependent callback Function invoked after crossing the Asynchronous Gap. The correct way to address this is to make this widget stateful:

GOOD:
class MyRetryWidget extends StatefulWidget {
  final Widget? child;
  final bool Function(BuildContext, String)? shouldRetry;
  final Function(BuildContext, String)? onRetryClicked;
  const MyRetryWidget({Key? key, this.child, this.shouldRetry, this.onRetryClicked});
  
  @override
  State<MyRetryWidget> createState() =>
      _MyRetryWidgetState();
}

class _MyRetryWidgetState extends State<MyRetryWidget> {
  @override
  Widget build(BuildContext context) {
    return BlocConsumer<MyBloc, MyState>(
      builder: (context, state) {
        final ThemeData currentTheme = Theme.of(context);
        final isDoingWork =
            state.stateType == MyStateType.doing_work;
        return ModalProgressHUD(
          inAsyncCall: isDoingWork,
          color: Colors.black87,
          progressIndicator: CircularProgressIndicator(
            valueColor:
                AlwaysStoppedAnimation<Color>(currentTheme.primaryColor),
          ),
          child: widget.child!,
        );
      },
      listener: (context, state) {
        switch (state.stateType) {
          case MyStateType.work_error:
            final bool shouldDisplayRetry =
                   widget.shouldRetry?.call(context, state.errorCode) ?? 
                   _shouldRetry(errorCode);
            final retry = await showDialog<bool>(
              barrierDismissible: false,
              context: context,
              builder: (context) {
                return PlatformAlertDialog(
                    title: errorTitle,
                    content: errorWidget,
                    actions: <Widget>[
                      PlatformDialogAction(
                          child: PlatformText(shouldDisplayRetry ? "No" : "OK"),
                          onPressed: () {
                            Navigator.of(context).pop<bool>(false);
                          }),
                      if (shouldDisplayRetry)
                        PlatformDialogAction(
                            child: PlatformText("Retry"),
                            onPressed: () {
                              Navigator.of(context).pop<bool>(true);
                            })
                    ],
                );
            }) ?? false;
            if (mounted && retry) {
              widget.onRetryClicked?.call(context, state.errorCode);
            }
            break;
          case MyStateType.success:
            Navigator.pop<bool>(context, true);
            break;
          default:
            break;
        }
      },
    );
  }
}

Finally, another pitfall that looks like an easy way out is the use of Future.then, which again, implicitly crosses the Asynchronous Gap:

BAD:
class MyStatelessWidget extends StatelessWidget {
 @override
 Widget build(BuildContext context){
  return ElevatedButton(
   onPressed: () async {
     Future<int>.value(42).then((_){
       Navigator.of(context).pop();
     });
   },
   child: const Text('button'),
  );
 }
}

It should be made abundantly clear in the documentation that using Future.then is not a substitute due to it implicitly crossing the Asynchronous Gap.

Thanks for opening this!

@goderbauer: would you be a good person to give feedback on the proposed additions?

@pq you're welcome.

I figured that other folks will run into the same issues that I ran into when I saw the related Decoding Flutter video. In other words, when using a stateful widget that provides callbacks to various state changes (BlocConsumer) in a StatelessWidget, there's no example to cover that edge case. I ended up making that StatelessWidget stateful since if I used Future.then, I would run into the same problem as the lint.

Is there already documentation about these kinds of issues around build contexts? If so, does that cover these kinds of cases?

I ask because it might be better for the lint's documentation to point to more complete documentation somewhere else rather than to try to duplicate all that information.

If there isn't documentation already, and if we don't want to add it, then we probably do want to add it here. But we'll need to pair down the examples as much as possible for readability.

@bwilkerson There were no examples where you're using a widget that provides a listener surface when using it in a StatelessWidget, (i.e. BlocConsumer), which is one of the scenarios that I ran into when re-enabling that lint and fixing the issues related to it in my code. In order to fix it, I faced two options: 1) use Future.then or 2) make that StatelessWidget stateful in order to use mounted.

I ended up picking the latter since the former would still be implicitly crossing the Asynchronous Gap, which this lint is trying to prevent. That and people might naturally use Future.then as an easy fix, when in reality, it doesn't fix it at all. The lint's documentation doesn't spell this out, since I foresee people going down this route thinking that they addressed the lint, when all they did was bury the issue into the ground like a landmine.

I'm not claiming that we don't need to document such cases, I'm just thinking about the best way / place to document them.

@bwilkerson IMHO, they should be added to the lint's existing description as supplementary examples, since this has the potential of creating landmines throughout one's code. The best way to avoid them is to warn the other person where they are more likely to read it: from the lint's description.

I agree that the current documentation on the lint is lacking a bit and could be improved. The suggested MyRetryWidget feels too complex for this, though. I think the documentation should contain examples that are easy/quick to comprehend.

I also agree with @bwilkerson that most of that information also belongs into Flutter's API docs somewhere. Not sure where people would look for this, though. Maybe we can add a section for this to BuildContext and link it from various places where people deal with context (i.e. from State.context and from the build methods that provide you with the context). The lint's documentation could then also link back to that section.

@goderbauer I can definitely scale down the MyRetryWidget example. I think the gap in documentation remains the same: if you have a StatelessWidget that injects a BuildContext to a Function, then there needs to be an example of how to handle it, by making it stateful.