search

GregoryConrad / rearch-dart

Re-imagined approach to application design and architecture
https://pub.dev/packages/rearch
MIT License
92 stars 4 forks source link

Listener isn't working #93

Closed busslina closed 9 months ago

busslina commented 9 months ago

I have:

class DataModule { void Function() rebuild;

DataModule({ required this.rebuild, });

late cm.Info _votingInfo;

cm.Info get votingInfo => _votingInfo;

void setVotingInfo(cm.Info info) { debug('DataModule.setVotingInfo');

_votingInfo = info;

debug('DataModule -- REBUILDING');
rebuild();

}

void _updateRebuild(void Function() newRebuild) { rebuild = newRebuild; } }


- IndexRoute: (I only show the methods `componentDidMount` and `componentWillUnmount`):
```dart
@override
  void componentDidMount() {
    super.componentDidMount();

    debug('IndexRoute -- SETTING LISTENER');

    // Listening to data module
    _cancelDataModuleListener = cc.listen((use) {
      final aa = use(dataModuleCapsule);

      debug('FORCING UPDATE');

      forceUpdate();
    });

    [...]
  }

@override
  void componentWillUnmount() {
    debug('IndexRoute.componentWillUnmount()');

    // Rearch listeners
    debug('IndexRoute -- CLOSING LISTENER');
    _cancelDataModuleListener();

    // Cancelling periodic timer
    _periodicTimer?.cancel();

    super.componentWillUnmount();
  }

The issue is that I'm updating the DataModule via its setVotingInfo() but the listener is not getting executed (it's only executed the first time, when it's setup).

It's weird because I use the same listener pattern in other parts and works well, with the only difference that the other capsules don't use rebuild() because they are using use.state.

Do you see any bug here?

Here it's the output on the browser:

[DEBUG] -- DataModule.setVotingInfo
[DEBUG] -- DataModule -- REBUILDING
[DEBUG] -- IndexRoute -- SETTING LISTENER
[DEBUG] -- FORCING UPDATE
[DEBUG] -- DataModule.setVotingInfo
[DEBUG] -- DataModule -- REBUILDING
[DEBUG] -- DataModule.setVotingInfo
[DEBUG] -- DataModule -- REBUILDING
[DEBUG] -- DataModule.setVotingInfo
[DEBUG] -- DataModule -- REBUILDING
[DEBUG] -- DataModule.setVotingInfo
[DEBUG] -- DataModule -- REBUILDING

As you can see, the listener is never closed (because the IndexRoute never gets unmounted)

GregoryConrad commented 9 months ago

with the only difference that the other capsules don't use rebuild() because they are using use.state.

use.state calls rebuild under the hood.

what does forceUpdate() do??

busslina commented 9 months ago

what does forceUpdate() do??

just updates the rebuilder. I did that because I wasn't sure the rebuilder is valid/persistent between rebuilds.

Anyways it fails too with this version:

DataModule dataModuleCapsule(CapsuleHandle use) {
  final rebuild = use.rebuilder();
  return use.value(DataModule(rebuild: rebuild));
}

class DataModule {
  void Function() rebuild;

  DataModule({
    required this.rebuild,
  });

  late cm.Info _votingInfo;

  cm.Info get votingInfo => _votingInfo;

  void setVotingInfo(cm.Info info) {
    debug('DataModule.setVotingInfo');

    _votingInfo = info;

    debug('DataModule -- REBUILDING');
    rebuild();
  }
}
GregoryConrad commented 9 months ago

Oh, I was thinking the in wrong direction. Your DataModule doesn’t have an == impl

Either:

  1. Make DataModule immutable and let ReArch use immutable data (preferred, and you won’t have to worry about rebuilds yourself)
  2. Add an == to DataModule that incorporates the voting info
busslina commented 9 months ago

Thanks, now it works great (again). Didn't knew that rebuild() also checks use.value values with ==.

part 'voting_data.freezed.dart';

(VotingData, void Function(VotingData)) _votingDataControllerCapsule(
  CapsuleHandle use,
) {
  return use.state(VotingData());
}

VotingData votingDataCapsule(CapsuleHandle use) =>
    use(_votingDataControllerCapsule).$1;

void Function(VotingData Function(VotingData)) votingDataUpdaterCapsule(
  CapsuleHandle use,
) {
  final (currentValue, setter) = use(_votingDataControllerCapsule);
  return (updater) => setter(updater(currentValue));
}

@freezed
class VotingData with _$VotingData {
  const factory VotingData({
    cm.Info? votingInfo,
  }) = _VotingData;
}
cc.read(votingDataUpdaterCapsule)(
        (currentValue) => currentValue.copyWith(votingInfo: newInfo));
GregoryConrad commented 9 months ago

Ahh, use.value just returns whatever was passed into it on the first build! use.lazyValue is just an alias to use.callonce, and use.value is a simple wrapper around use.lazyValue. Since all capsules use == to check for rebuilds, if you return something directly from use.value, the new data always be equal to the old data (unless you override ==)

as far as the last code snippet you provided: you probably would be better suited by having votingDataUpdaterCapsule return a function that takes in the votingInfo and call the copyWith internally to make it easier. Or, if your situation is more complicated, maybe use.reducer.

busslina commented 9 months ago

Having a rebuild seems like an alias to forceRebuild, because if using use.state, it will autorebuild on setState. I understand it purpose is to rebuild when no setState is involved and a not use.value is returned.

But do you think it will be good to have an option to force rebuild (without checking anything, just rebuild) like React's force update?

https://www.geeksforgeeks.org/reactjs-forceupdate-method/

GregoryConrad commented 9 months ago

Yea, exposing rebuild as it was is a point of confusion for library users--I probably shouldn't have shipped it like that. I tried to fix this later on with an optional parameter that takes in a function for backwards compatibility, but if I could do things over again, I would've made it a required parameter so it'd be easy to tell when you do and don't need rebuild. I also probably would change SideEffectRegistrar.register to make it easier to understand/use, without even ever having to worry about how rebuilds work (the term rebuild wouldn't be found anywhere in the public API). The folks at Flutter actually had the exact same problem with setState in the State class; see https://api.flutter.dev/flutter/widgets/State/setState.html#design-discussion

But do you think it will be good to have an option to force rebuild (without checking anything, just rebuild)

No, that feels hacky to me. If you are laying things out properly, you shouldn't need something like that. The issue is this can make app migration to ReArch harder, and I should probably add a blurb to the docs about migration (https://github.com/GregoryConrad/rearch-docs/issues/8). Apps in general probably shouldn't be migrated to ReArch the more I think about it, but rather be written from the ground up. It's a bit too different from other styles of development.

busslina commented 9 months ago

Apps in general probably shouldn't be migrated to ReArch the more I think about it, but rather be written from the ground up. It's a bit too different from other styles of development.

I agree 💯