letsar / binder

A lightweight, yet powerful way to bind your application state with your business logic.
MIT License
178 stars 12 forks source link

Widget watching on Computed is sometimes not rebuilt #18

Open Delgan opened 3 years ago

Delgan commented 3 years ago

Hey @letsar!

So, I was doing some tests using your excellent package, and I noticed a very weird bug. Basically, I have one widget watching a Computed reference which itself is based on a State reference. Each time the button is pressed, the state is updated and the other reference is re-computed. So far so good.

However, the widget is not systematically rebuilt. It only works half the time, randomly. Yet, the value returned by the Computed is definitely different!

https://user-images.githubusercontent.com/4193924/120108355-1f8b1b80-c165-11eb-8f40-6865a460d624.mp4

import 'package:binder/binder.dart';
import 'package:flutter/cupertino.dart';
import 'package:flutter/material.dart';

void main() => runApp(BinderScope(child: AppView()));

class AppView extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Column(
        mainAxisAlignment: MainAxisAlignment.center,
        children: [
          RaisedButton(
            child: Text("Press me"),
            onPressed: () => context.use(testLogicRef).click(),
          ),
          Builder(
            builder: (context) {
              final val = context.watch(computedValRef);
              print("Rebuilt with: $val");
              return Text("$val");
            },
          )
        ],
      ),
    );
  }
}

final testLogicRef = LogicRef((scope) => TestLogic(scope));

final testStateRef = StateRef(0);

final computedValRef = Computed<int>((watch) {
  final val = watch(testStateRef);
  final computedVal = DateTime.now().millisecondsSinceEpoch;
  print("Computed value: $computedVal");
  return computedVal;
});

class TestLogic with Logic {
  const TestLogic(this.scope);

  @override
  final Scope scope;

  void click() => update(testStateRef, (int val) => val + 1);
}

It drives me crazy. Am I doing something wrong?

Also, note that when I press the button once, the Computed is called twice or thrice. Is that expected?

I'm using Binder 0.4.0 with Flutter 2.0.6 and Dart 2.12.3.

erf commented 3 years ago

Hi,

I took a look at your example as i'm using Binder now in a live app, so this made me a bit concerned.

The behavior of your example does seem a bit strange.

If i simplify your example and only return val (the counter value) in computedValRef, the Text is always rebuilt. However, i do notice some strange behavior here. The old value is first returned in Computed, before the new value is returned twice.

If i instead return the computedVal (the time value), i noticed when it is generated twice with the same value, the widget is not rebuilt. Even if it is different than the previous clicked value.

Se my example below and try to comment out val in computedVal.

So to summarize, there seem to be two problems with Computed:

  1. The value is generated too often. First once with the previous value, then twice with the new. I would expect only the latest value.
  2. If two successive equal values are returned from Computed, the widget is not rebuilt.
import 'package:binder/binder.dart';
import 'package:flutter/cupertino.dart';
import 'package:flutter/material.dart';

void main() => runApp(const BinderScope(child: AppView()));

class AppView extends StatelessWidget {
  const AppView({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Column(
        mainAxisAlignment: MainAxisAlignment.center,
        children: [
          ElevatedButton(
            child: const Text("Press me"),
            onPressed: () => context.use(testLogicRef).click(),
          ),
          Builder(
            builder: (context) {
              final val = context.watch(computedValRef);
              debugPrint("Rebuilt with: $val");
              return Text("$val");
            },
          )
        ],
      ),
    );
  }
}

final testLogicRef = LogicRef((scope) => TestLogic(scope));

final testStateRef = StateRef(0);

final computedValRef = Computed<int>((watch) {
  final val = watch(testStateRef);
  debugPrint('Computed watch val $val');
  final int timeValue = DateTime.now().millisecondsSinceEpoch;
  debugPrint("Computed watch timeValue: $timeValue");
  return val;
  return timeValue;
});

class TestLogic with Logic {
  const TestLogic(this.scope);

  @override
  final Scope scope;

  void click() => update(testStateRef, (int val) {
        final int newVal = val + 1;
        debugPrint('click');
        debugPrint('TestLogic update with newVal $newVal');
        return newVal;
      });
}
Delgan commented 3 years ago

Thanks @erf for taking a closer look at this!

I came to the same conclusion. The behavior depends whether or not the same value is returned twice. I have no idea what could cause this in the Computed implementation.

erf commented 3 years ago

@Delgan we could try to write some tests for this

letsar commented 3 years ago

Hi, sorry for the delay, I'm very busy nowadays.

For the moment, the state of Computed is not cached, this is why the function inside the Computed is executed 3 times:

I didn't looked how to cache the values for the moment, but this is the next thing I want to achieve, so that it will be more efficient.

The purpose of Computed is to create a new data derived from one or more other values. In the example mentioned in the issue, the result of the Computed is not derived from the watched value, that's why you observe strange behaviors. It's not intended to be used like that.

In the example, the old and the new values will be too close from each other, since we don't cache the values but getting them with the old context and then with the new context. Since the two calls are next to each others I suspect that millisecondsSinceEpoch returns the same value for the old and the new values sometimes. Binder don't see any changes, so the widget is not rebuilt.

I hope this give you enough details to understand the situation.

erf commented 3 years ago

Hi, no worries. Caching sounds great - if it also will work for complex objects.

letsar commented 3 years ago

As long as objects are still immutable, it should also work

Delgan commented 3 years ago

Hi @letsar, thanks for the update!

To clarify, I came to the millisecondsSinceEpoch example by narrowing down the problem step by step. However, in my code, the value which initially triggered the issue looks as follows:

class SomeClass {
    final List<int> someValues;
    SomeClass(this.someValues);
}

final baseRef = StateRef(Map<DateTime, SomeClass>());

final derivedRef = Computed<Map<DateTime, int>>((watch) {
  final base = watch(baseRef);
  final derived = Map<DateTime, int>();
  base.forEach((key, value) {
    derived[key] = value.someValues.length;
  });
  return derived;
});

final computedValRef = Computed<int>((watch) {
  final derived = watch(derivedRef);
  final now = DateTime.now();
  var date = DateTime(now.year, now.month, now.day);
  var count = 0;
  while (derived.containsKey(date) && derived[date] > 0) {
    date = date.subtract(Duration(days: 1));
    count++;
  }
  return count;
});

The computedValRef is sort of derived from the others. I also make sure to create a new object each time baseRef is updated.

I guess the reported problem may occur if baseRef is updated several times during a short interval? Perhaps it is related to the parent context.

Anyway, I may have abused Computed values due to how convenient they are. :slightly_smiling_face: I will try to keep things simple and favor StateRef.