Possible bug

[busslina]

Hi again,

the other day you modified lazyStateGetterSetter to delay initial state resolving to the first getter execution. I changed all my rawValueWrapper (or almost) in favor of lazyStateGetterSetter. But I'm encountering problems in all (or almost) of them, like they refuse to update (on easy cases). I don't know if it is my error (I think no) or maybe yours in that modification.

Thanks

[busslina]

Gonna test it with old versions of ReArch. Now it is a little mess, each package using a different version. So I will unify them and check it.

[GregoryConrad]

Would you mind sharing a reproducible example?

[busslina]

So based on the test mentioned here I have this output:

The last line should show a null value. That is what is happening to my real project. No way to update my cachedSerializableValue side effect either reactive or imperative way. It was working well with rawValueWrapper. I adapted it to lazyStateGetterSetter and it's not working:

/// If [doReset] is true it will reset the cache at build time.
CachedSerializableValueController<T>
    cachedSerializableValue<T extends Object>(
  T? initial, {
  bool doReset = false,
  String? debugResetFlag,
}) {
  final (getState, setState) = use.lazyStateGetterSetter<T?>(() => initial);
  final (getVersion, setVersion) = use.lazyStateGetterSetter(() => 0);

  final transactionRunner = use.transactionRunner();

  bool reset() {
    debug(
      'cachedSerializableValue -- reset -- I -- $debugResetFlag -- ${getState()}',
      active: debugResetFlag != null,
    );

    if (getVersion() == 0) {
      return false;
    }

    debug(
      'cachedSerializableValue -- reset -- II -- $debugResetFlag -- ${getState()}',
      active: debugResetFlag != null,
    );

    transactionRunner(() {
      setVersion(0);
      setState(initial);
    });

    return true;
  }

  final version = getVersion();

  final ctrl = CachedSerializableValueController<T>(
    value: getState(),
    version: version,
    fresh: (freshState) {
      transactionRunner(() {
        setVersion(0);
        setState(freshState);
      });
    },
    set: (newState, newVersion) {
      if (newVersion <= version) {
        return false;
      }

      transactionRunner(() {
        setVersion(newVersion);
        setState(newState);
      });

      return true;
    },
    setIncremental: (newState) {
      transactionRunner(() {
        setVersion(version + 1);
        setState(newState);
      });

      return true;
    },
    setVersionedValue: (versionedValue) {
      if (versionedValue.version <= version) {
        return false;
      }

      transactionRunner(() {
        setVersion(versionedValue.version);
        setState(versionedValue.value as T);
      });

      return true;
    },
    setVersionedValueDef: (vesionedValueDef) {
      if (vesionedValueDef.version <= version) {
        return false;
      }

      transactionRunner(() {
        setVersion(vesionedValueDef.version);
        setState(vesionedValueDef.value);
      });

      return true;
    },
    reset: reset,
    modified: () => version != 0,
  );

  // Conditionally reset
  if (doReset) {
    reset();
  }

  return ctrl;
}

[busslina]

test.zip

[GregoryConrad]

I looked at the zip but not completely following what is happening. If you can share a minimal reproducible example with just ReArch code, that'd help a lot.

No way to update my cachedSerializableValue side effect either reactive or imperative way.

If that is the issue, have the capsule that calls use.cachedSerializableValue also use(myDepCapsule) so that it'll get updates when myDepCapsule changes. Just like how you're doing:

use.cachedSerializable(
  doReset: use(shouldBeResetCapsule),
);

Maybe you're missing something like that in another capsule?

[GregoryConrad]

(Accidentally closed)

[busslina]

I simplified it: removed external dependencies, side effect simplified, now using typedef instead of class as capsule controller and upgraded Rearch to version 1.7.1. You must have no problem to test it.

Same problem: resetting capsule to the initial value is not working: test_possible_bug.zip

* I don't think it is an error introduced in the last lazyStateGetterSetter code modification because it is behaving equally with latest and previous versions.

[busslina]

Digging more, I added some debug prints in your lazyStateGetterSetter code. Seems that api.rebuild is not executing the provided callback. There is the trouble:

/// Side effect that provides a way for capsules to contain some state,
/// where the initial state is computationally expensive.
/// Further, instead of returning the state directly, this instead returns
/// a getter that is safe to capture in closures.
/// Similar to the `useState` hook from React;
/// see https://react.dev/reference/react/useState
(T Function(), void Function(T)) lazyStateGetterSetter<T>(T Function() init) {
  // We use register directly to keep the same setter function across rebuilds
  // (but we need to return a new getter on each build, see below for more)
  final (getter, setter) = use.register((api) {
    var hasBeenInit = false;
    late T state;

    T getter() {
      if (!hasBeenInit) {
        state = init();
        hasBeenInit = true;
      }
      return state;
    }

    void setter(T newState) {
      print('lazyStateGetterSetter -- setter -- 1');
      api.rebuild((cancelRebuild) {
        print('lazyStateGetterSetter -- setter -- 2');
        if (hasBeenInit && newState == state) {
          print('lazyStateGetterSetter -- setter -- 3');
          cancelRebuild();
          return;
        }

        print('lazyStateGetterSetter -- setter -- 4');

        state = newState;
        hasBeenInit = true;

        print('lazyStateGetterSetter -- setter -- 5 -- $state');
      });
    }

    return (getter, setter);
  });

Output:

[busslina]

And, ran in debug mode (dart run --enable-asserts ...) I got this:

So, either it is not a good practice to call a setState from with in a capsule (I asked you here and seemed okay) or there is something wrong in the implementation (related to _debugCanCallRunTxn and runTransaction).

[busslina]

So, if I'm correct in the previous message, the solution would be to do similar as persist, which do not await read, so it wont trigger rebuilds on the current "loop". I did a test by wrapping my reset function into Future.microtask and works well (I don't like that it rebuilds twice). But if it's the finest solution I will adopt it.

[GregoryConrad]

Ohh, I have no clue how I didn’t see this. It’s because if(doReset) calls reset() which sets the states/runs a txn and invokes a rebuild within the build.

You can’t do that atm. I’ve toyed with the idea of allowing it for side effects that return a getter instead of a concrete value (like stateGetterSetter but not just state), but it seems like it’d be too complicated.

I’m surprised you didn’t get an assert telling you to not invoke rebuilds inside a build. I’ll have to look into why that didn’t happen later.

in the meantime, this particular situation is probably best handled by rawValueWrapper in conjunction with use.rebuilder. Wondering if I could make a new side effect that’d make that combo a little easier since it is needed sometimes (like here).

[GregoryConrad]

Oops, I only responded to your first message. Just read the rest of them. Yea, that assert you got is correct. I missed the transaction running in reset that gets called during build (from your discussion post), so sorry about that.

[busslina]

Don't worry :)

So what would be your solution for this situation?

• Wrap the transaction with Future.microtask? (Or make the reset function async)
• Reset data capsules imperatively on websocket disconnection?
• Any other workaround?

* Just saw that you answered already

[busslina]

in the meantime, this particular situation is probably best handled by rawValueWrapper in conjunction with use.rebuilder. Wondering if I could make a new side effect that’d make that combo a little easier since it is needed sometimes (like here).

So use.rebuilder is safe to execute at build time??

[GregoryConrad]

Wrap the transaction with Future.microtask? (Or make the reset function async)

This would be equivalent to getting around calling the good-ol setState during a widget rebuild in Flutter. It works, but is really a hack more than it is a solution.

Reset data capsules imperatively on websocket disconnection?

I'd advise against that as well.

Any other workaround?

Yea, at the moment, it'd be rawValueWrapper coupled with rebuilder. Those are both relatively more low-level and consequently not a great combo to use in regular app-level code (i.e., not used internally in ReArch), so I may want to explore the idea of a new side effect that wraps around them to make mutations during build more ergonomic.

You can also trying using use.register directly here--it may be a good fit. It gives you no building blocks to use out-of-the-box, but that also allows you to write things exactly as you want.

So use.rebuilder is safe to execute at build time??

Wasn't clear enough here. You need to:

• Have a getter + setter to be able to get/set the value without rebuilds (which will allow you to call the setter during the build itself)
• Return a new instance of the getter each build, along with a setter that wraps around the setter above by calling it inside a rebuild.

And both of the above is exactly what the new side effect I am thinking would do:

final (value, setValue, mutateValue) = use.mutableValue(fooBar);
// safe to call mutateValue in the capsule directly, but not outside build
// safe call setValue outside build but not in capsule

And, to understand some of my thinking, I was at one point thinking about making use.state et al. return a ValueWrapper or something similar, like wrapper.value = 123; print(wrapper.value);. Then, we could do away with the notion of rebuilds and you could update capsule values directly within the build and ReArch could figure out exactly when to do everything on its own.

But, I thought this would make function composition harder; i.e., if I want to make an intermediary capsule between some stateful capsule and an action that emits a log every time the state is set, it may get a lil iffy because you're combining the .value = newValue, which is a statement, alongside other function calls.

considering Dart doesn't really have any safety around when variables support interior mutability/not, this might have actually been the best approach. I could do it in a non-breaking release, but frankly it'd clash with the current ways of doing things and would be better to just do a v2.

Also now that I've said all that, I realize I haven't even given the core of the problem. Take this:

final (state, setState) = use.state(0);
setState(123);
// state would still be 0 on this build.

That's obviously not ideal and why it is currently disallowed. This is solved when using getters/setters instead of referencing the variable directly, or the ValueWrapper, but I didn't do that.

In Rust this was not even an issue since a capsule is given a &mut of the side effect during build, which makes it obvious what is going on, but then has to invoke a callback to get a &mut outside of the build, which then triggers a rebuild. Unfortunately, Dart doesn't have anything like &mut, and thus the issue.

[GregoryConrad]

The more I think about it, the more I feel like ValueWrapper would've been the right approach... oops. Wonder if a v2 would be worth it just for that, especially since all "rebuild" terminology would be thrown out the window as it'd be no longer relevant.

Maybe I could do this in a backward-compatible way. I can try that later today.

[busslina]

Don't rush for me. It would be great to have that side effect. For me it doesn't matter if you "break" your lib. All we can just use and old version and try to adapt us to the newer one. Also, with each problem I have here I look more over the code and I think that little by little I understand it more.

That's obviously not ideal and why it is currently disallowed. This is solved when using getters/setters instead of referencing the variable directly, or the ValueWrapper, but I didn't do that.

Yeah, seems use.state breaks FP by having a value instead a getter. It could be removed in favour of a full FP side effect. Then we can do a custom "final" side effect that returns a value...

But, I thought this would make function composition harder

And if removing current use.state helps on this, then it's a win win.

If you want, you can do this job on a secondary git branch and I can give you feedbacks