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

fix: Cubit (but not Bloc) state behavior is incorrect during 1st frame build #3693

Open
mgj opened this issue Jan 19, 2023 · 5 comments
Open
Assignees
Labels
bug Something isn't working investigating Investigating the issue

Comments

@mgj
Copy link

mgj commented Jan 19, 2023

Description
I understand that some emitted states may be ignored/unhandled by the View/Page if the View is already processing/rendering a previously emitted state ( #459 ). Example:

Emit State1
Emit State2
Emit State3

Where Only State1 and State3 are actually received by the View. This is fine. However, it is possible to create a situation in which only State1 is received by the View and both State2 and State3 are lost (!). This means that the View never "catches up" to the current state.

It is possible to work around this but it seems like a very dangerous and hard-to-find pitfall. You will have tests verifying the business logic of the Cubit but then once the code is deployed, it no longer works as expected.

Steps To Reproduce

I've created a minimal example here: https://github.com/mgj/flutter_bloc_cubit_issue

The CubitBasedPage replicates this seemingly broken behavior, where the CubitBasedReadyState is never reached.
The BlocBasedPage uses Bloc instead of Cubit and does not replicate the behavior.
The CubitBasedWorkaroundPage shows an interesting workaround where, by waiting for the first frame to be fully rendered before emitting new states, the behavior is not replicated. It is strange to me that after the first frame, this behavior is no longer replicable - almost suggesting an issue in when/how the Page subscribes to the event stream.

Expected Behavior
Expected to have one or more states "in the middle" of the stream be ignored, but the final/last state emitted never being ignored by the View

Screenshots
No screenshots

Additional Context
It seems that calling init() from the Cubit constructor does resolve the issue but there are 2 problems with that:

  1. Makes the Cubit less testable
  2. Is it actually a reliable, trustworthy, solution, or is it just an "accident" that the timing works out between the View and the Cubit in that case?

Thanks in advance, please let me know if there is any additional information I can provide

@mgj mgj added the bug Something isn't working label Jan 19, 2023
@felangel felangel self-assigned this Jan 19, 2023
@felangel felangel added the investigating Investigating the issue label Jan 19, 2023
@jacobtipp
Copy link

jacobtipp commented Jan 20, 2023

@mgj

Instead of calling init() from the BlocBuilder, you should call it in the BlocProvider's create method.

In your example it would look like this:

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: BlocProvider(
        create: ((context) => CubitBasedCubit()..init()), // <--- initialization events and actions 
        child: const CubitBasedPage(),
      ),
      /*
      home: BlocProvider(
        create: ((context) => BlocBasedCubit()),
        child: const BlocBasedPage(),
      ),
     */
      /*
      home: BlocProvider(
        create: ((context) => CubitBasedWorkaroundCubit()),
        child: const CubitBasedWorkaroundPage(),
      ),
      */
    );
  }
}

@tenhobi
Copy link
Collaborator

tenhobi commented Jan 22, 2023

Hej @mgj, generally speaking I would say that you should never put logic like this (non-ui) in your build method, as you are doing here https://github.com/mgj/flutter_bloc_cubit_issue/blob/main/lib/pages/cubitbased/cubitbased_page.dart#L19

  @override
  Widget build(BuildContext context) {
    return Scaffold(body: BlocBuilder<CubitBasedCubit, CubitBasedState>(
      builder: (context, state) {
        final cubit = context.read<CubitBasedCubit>();
        if (cubit.state is CubitBasedInitialState) {
          cubit.init(); // don't do this
          return const CircularProgressIndicator();
        } 
        // ...
      },
    ));

Your build method should only build a widget according to its state, not communicate with blocs and other services. One reasoning behind that might be that the build method might never be called at all, might be called once, might be called multiple times, and that will make your app unpredictable.

If you want to listen for your state changes, use listen method in BlocListener or BlocConsumer. In this callback method, you can listen to cubit state changes and add events (calling cubit methods) to the bloc.

As for the calling of the init() method, you can call it from the create callback of BlocProvider as @jacobtipp suggests, as it is the perfect time and place to call such an event.

@mgj
Copy link
Author

mgj commented Jan 23, 2023

Thanks @jacobtipp and @tenhobi for taking the time to respond, I appreciate it.

I understand there are workarounds for this issue but that does not mean that there is not still a problem - It should never be possible to get into a situation where the Page is not notified of the last/final emitted State.

As for using BlocListener its not actually possible to do that for initialization since the listen in BlocListener is only called on state changes, not for the initial state.

@tenhobi
Copy link
Collaborator

tenhobi commented Jan 23, 2023

Hmm, the behaviour of Cubit vs Bloc in matter of what will be logged is indeed different. But that is, I suppose, related to from which context you trigger the event. One is from a synchronous context, aka when using Cubits. Second is from asynchronous event, aka when using Bloc.

Using sinks for event (bloc) instead of synchronous function (cubit) makes a bit of "delay" which you can somehow "simulate" using scheduleMicrotask or SchedulerBinding.instance.addPostFrameCallback:

builder: (context, state) {
  final cubit = context.read<CubitBasedCubit>();
    if (cubit.state is CubitBasedInitialState) {
    log("builder - initial");
    scheduleMicrotask(() {
      cubit.init();
    });
    return const CircularProgressIndicator();
  }
  // ...
}

Also, when you write a test, you would discover that all the event emitted are the same no matter if that is a bloc or cubit:

blocTest<CubitBasedCubit, CubitBasedState>(
  'TODO: description',
  build: () => CubitBasedCubit(),
  act: (bloc) async {
    await bloc.init();
  },
  expect: () => <CubitBasedState>[
    CubitBasedLoadingState(),
    CubitBasedReadyState(),
  ],
);

with output:

Expected: [Instance of 'CubitBasedLoadingState', Instance of 'CubitBasedReadyState']
Actual: [Instance of 'CubitBasedLoadingState', Instance of 'CubitBasedReadyState']

As I said, using create: ((context) => CubitBasedCubit()..init()) is not a workaround and is the right way to do the task. More thoughts:

  1. Calling cubit directly from builder method is not safe, because it can be called never, once, or even multiple times. All depending on the UI layer needs.
builder: (context, state) {
  if (cubit.state is CubitBasedInitialState) {
    cubit.init(); // <--- don't do this
    return const CircularProgressIndicator();
  }
  // ...
}
  1. create callback in Provider is the right time and place to init the bloc, because you are creating and initialiazing the bloc there.

  2. Because of 1, you have to work with your state, no matter if from bloc or cubit, as if you have no clue how many states and how many times each were actually rendered. Widget is a function of state. All the logic should be in your bloc.

  3. If do not want to call init() in create, for some reason, you have to pass the cubit as a parameter to the stateful widget and in the initState method, you have to schedule async event (so your widget is built before running the init() method and therefore can listen to changes) where you call the initialisation.

  @override
  void initState() {
    SchedulerBinding.instance.addPostFrameCallback((timeStamp) {
      widget.cubit.init();
    });
    super.initState();
  }

@jacobtipp
Copy link

Thanks @jacobtipp and @tenhobi for taking the time to respond, I appreciate it.

I understand there are workarounds for this issue but that does not mean that there is not still a problem - It should never be possible to get into a situation where the Page is not notified of the last/final emitted State.

As for using BlocListener its not actually possible to do that for initialization since the listen in BlocListener is only called on state changes, not for the initial state.

The problem would be performing side-effect code inside a build callback, that's what creates the unpredictable behavior that you are seeing. If you are familiar with react, it would be the equivalent of calling setState inside a components render method.

Calling cubit.init() inside of a build callback is performing a side-effect.

Referring to flutter docs on the build method:

The framework calls this method when this widget is inserted into the tree in a given BuildContext and when the dependencies of this widget change (e.g., an InheritedWidget referenced by this widget changes). This method can potentially be called in every frame and should not have any side effects beyond building a widget.

Please refer to this stackoverflow answer as well:

https://stackoverflow.com/a/62652122

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working investigating Investigating the issue
Projects
None yet
Development

No branches or pull requests

4 participants