brianegan / flutter_redux

A library that connects Widgets to a Redux Store
MIT License
1.65k stars 219 forks source link

Socket and Flutter_redux #242

Closed barilki closed 1 year ago

barilki commented 1 year ago

Hi, I'm using signal_core and flutter redux. I have a problem with the dispatch inside the socket service. Each time I get changes from socket and dispatch them, all the pages are rebuilt. How can I prevent this from happening?

brianegan commented 1 year ago

You need to use a StoreConnector Widget + the set distinct to true.

You can read more about it from the docs:

Every time the store changes, the Widget will be rebuilt. As a performance optimization, the Widget can be rebuilt only when the ViewModel changes. In order for this to work correctly, you must implement == and hashCode for the ViewModel, and set the distinct option to true when creating your StoreConnector.

And see an example here:

https://github.com/brianegan/flutter_architecture_samples/blob/master/redux/lib/containers/stats.dart

barilki commented 1 year ago

@brianegan I've been implement it and still the pages get rebuilt each time.

barilki commented 1 year ago

Every time I dispatch an action from the socket service, all my pages are rebuilt. i'm using distinct and == operator inside view model.

brianegan commented 1 year ago

Could you share the code that isn't working, or at least a small reproducible sample? I've shared a working example above. Is something similar not working?

Distinct + == are tested, but there could still be a bug. Impossible for me to help without seeing some code.

https://github.com/brianegan/flutter_redux/blob/master/test/flutter_redux_test.dart#L626

barilki commented 1 year ago

For example:

setting page with setting view model

class NotificationsPageVM { NotificationsPageVM({ this.notifyList, this.notifyListFromServer, this.allInstruments, this.removeFromNotif, this.getAlertsList, this.notifications, });

final List? notifyList; final List? allInstruments; final List<GetAlertsRespDto?>? notifyListFromServer;

final List? notifications;

final void Function(String id)? removeFromNotif; final void Function()? getAlertsList;

static NotificationsPageVM fromStore(Store store) { return NotificationsPageVM( notifyList: store.state.notificationState.activeNotification, allInstruments: store.state.symbolState.allSymbols, notifyListFromServer: store.state.notificationState.activeNotificationList, removeFromNotif: NotificationSelector.getRemoveFromNotificationsFunction(store), getAlertsList: NotificationSelector.getAlertsList(store), notifications: NotificationSelector.getNotificationsList(store), ); }

@override bool operator ==(Object other) => identical(this, other) || other is NotificationsPageVM && runtimeType == other.runtimeType && notifyList == other.notifyList && notifyListFromServer == other.notifyListFromServer && allInstruments == other.allInstruments && removeFromNotif == other.removeFromNotif && getAlertsList == other.getAlertsList;

@override int get hashCode => notifyList.hashCode ^ notifyListFromServer.hashCode ^ allInstruments.hashCode ^ removeFromNotif.hashCode ^ getAlertsList.hashCode; }

this is the small part of instrument page (Storeconnector) @override Widget build(BuildContext context) { return MainLayout( key: Key('[NotificationsPage][MainLayout]'), buildPhoneLayout: (BuildContext context, bool isSmallPhone) { return StoreConnector<AppState, NotificationsPageVM>( distinct: true, converter: NotificationsPageVM.fromStore, builder: (BuildContext context, NotificationsPageVM vm) { .... }

in the main i dispatch event inside timer every second Timer.periodic( Duration(milliseconds: SocketConsts.INSTRUMENTS_TIMER_DELAY_INMILLISECONDS), () { widget.store.dispatch(UpdateSymbolsWithSocketDataAction( symbols: symbols, )); }, );

As a result, the setting page and other pages are always rebuilt.

brianegan commented 1 year ago

Thanks. Do you have tests that verify the == method on the NotificationsPageVM is working as expected?

I could see a couple of issues here.

  1. By default, Dart does not compare List objects by content, but by identity. Therefore, if you're creating a new notifyList, allInstruments, notifyListFromServer or notifications list inside your reducer, the == method will assume these are two different objects. Many folks use the collection package's ListEquality class to compare the content of two lists.
  2. Dart also compares Function objects only by identity. It looks like your callback functions are created by a selctor, so I assume that's working as intended, but it'd still be good to verify with a test case.

First, I'd start by trying the following test to make sure the == method is working as expected. If the following test works, please let me know and we can dig deeper. However, this feels like something is going on with the == method, since that's all the distinct option does: compare the old ViewModel to the new ViewModel using the == operator.

test('NotificationsPageVM == works as expected', () {
  final store = createStore(); // Example function, should be replaced by how you're building your store
  final viewModel = NotificationsPageVM.fromStore(store);

  store.dispatch(UpdateSymbolsWithSocketDataAction(symbols: symbols));

  final newViewModel = NotificationsPageVM.fromStore(store);

  expect(viewModel.notifyList, equals(newViewModel.notifyList));
  expect(viewModel.allInstruments, equals(newViewModel.allInstruments));
  expect(viewModel.notifyListFromServer, equals(newViewModel.notifyListFromServer));
  expect(viewModel.notifications, equals(newViewModel.notifications));
  expect(viewModel.removeFromNotif, equals(newViewModel.removeFromNotif));
  expect(viewModel.getAlertsList, equals(newViewModel.getAlertsList));
  expect(viewModel, equals(newViewModel));
});
brianegan commented 1 year ago

Looking at your code for a little longer, is the symbols List dispatched by the Timer.periodic( always a new List? If so, I'd imagine that might be the root of the problem.

barilki commented 1 year ago

class NotificationsPageVM{ NotificationsPageVM({ this.notifyList, this.notifyListFromServer, this.allInstruments, this.removeFromNotif, this.getAlertsList, this.notifications, });

final List? notifyList; final List? allInstruments; final List<GetAlertsRespDto?>? notifyListFromServer;

final List? notifications;

final void Function(String id)? removeFromNotif; final void Function()? getAlertsList;

static NotificationsPageVM fromStore(Store store) { return NotificationsPageVM( notifyList: store.state.notificationState.activeNotification, allInstruments: store.state.symbolState.allSymbols, notifyListFromServer: store.state.notificationState.activeNotificationList, removeFromNotif: NotificationSelector.getRemoveFromNotificationsFunction(store), getAlertsList: NotificationSelector.getAlertsList(store), notifications: NotificationSelector.getNotificationsList(store), ); }

@override bool operator ==(Object other) { if (identical(this, other)) return true;

return other is NotificationsPageVM &&
    listEquals(other.notifyList, notifyList) &&
    listEquals(other.notifyListFromServer, notifyListFromServer) &&
    listEquals(other.allInstruments, allInstruments) &&
    listEquals(other.notifications, notifications) &&
    other.removeFromNotif == removeFromNotif &&
    other.getAlertsList == getAlertsList;

}

@override int get hashCode { return notifyList.hashCode ^ notifyListFromServer.hashCode ^ allInstruments.hashCode ^ notifications.hashCode ^ removeFromNotif.hashCode ^ getAlertsList.hashCode; } }

Still the same, notification page are always rebuilt

brianegan commented 1 year ago

Thanks. Did the test case I sent through pass as well?

On Sun, Mar 12, 2023 at 00:12 Bar-Ilan Kimbarovski @.***> wrote:

class NotificationsPageVM{ NotificationsPageVM({ this.notifyList, this.notifyListFromServer, this.allInstruments, this.removeFromNotif, this.getAlertsList, this.notifications, });

final List? notifyList; final List? allInstruments; final List<GetAlertsRespDto?>? notifyListFromServer;

final List? notifications;

final void Function(String id)? removeFromNotif; final void Function()? getAlertsList;

static NotificationsPageVM fromStore(Store store) { return NotificationsPageVM( notifyList: store.state.notificationState.activeNotification, allInstruments: store.state.symbolState.allSymbols, notifyListFromServer: store.state.notificationState.activeNotificationList, removeFromNotif: NotificationSelector.getRemoveFromNotificationsFunction(store), getAlertsList: NotificationSelector.getAlertsList(store), notifications: NotificationSelector.getNotificationsList(store), ); }

List showNotif() { final List sorted = [];

for (var favId in notifyList!) { for (var inst in allInstruments!) { if (favId == inst.name) { sorted.add(inst); } } } return sorted;

}

@OverRide https://github.com/OverRide bool operator ==(Object other) { if (identical(this, other)) return true;

return other is NotificationsPageVM && listEquals(other.notifyList, notifyList) && listEquals(other.notifyListFromServer, notifyListFromServer) && listEquals(other.allInstruments, allInstruments) && listEquals(other.notifications, notifications) && other.removeFromNotif == removeFromNotif && other.getAlertsList == getAlertsList;

}

@OverRide https://github.com/OverRide int get hashCode { return notifyList.hashCode ^ notifyListFromServer.hashCode ^ allInstruments.hashCode ^ notifications.hashCode ^ removeFromNotif.hashCode ^ getAlertsList.hashCode; } }

Still the same, notification page are always rebuilt

— Reply to this email directly, view it on GitHub https://github.com/brianegan/flutter_redux/issues/242#issuecomment-1465120791, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA65DBO6RJRHH6MYE4WDKLW3WANTANCNFSM6AAAAAAVB6LZCE . You are receiving this because you were mentioned.Message ID: @.***>

barilki commented 1 year ago

Yes test case passed

barilki commented 1 year ago

Your assistance would greatly be appreciated. I need it urgently, thanks.

brianegan commented 1 year ago

I'm on vacation visiting family and friends and helping out in my free time. Please do not expect an urgent level of support.

Since I do not have full access to your code, it's impossible for me to say where the problem is.

At this point, I'd start doing more debugging and logging:

  1. Put a print statement outside the builder and inside the builder to make sure the widget isn't being rebuilt at a higher level.
  2. Debug the == method with the interactive debugger.
  3. Debug the line I linked to above where distinct is used.
  4. Debug the whole stream chain in the flutter_redux library.

Since you're the only one with access to the complete source code, only you can step through to determine where the issue lies. It could be with this library or with your code. I can't say without more information and help from you to debug more thoroughly to find the root cause.

On Tue, Mar 14, 2023 at 00:02 Bar-Ilan Kimbarovski @.***> wrote:

Your assistance would greatly be appreciated. I need it urgently, thanks.

— Reply to this email directly, view it on GitHub https://github.com/brianegan/flutter_redux/issues/242#issuecomment-1467484998, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA65DHN4PQK2F4Y6HSY4ULW4AJY3ANCNFSM6AAAAAAVB6LZCE . You are receiving this because you were mentioned.Message ID: @.***>

barilki commented 1 year ago
  1. widget isn't rebuild outside the builder, only inside store connector builder.
  2. == method return true.
  3. bool _whereDistinct(ViewModel vm) { if (widget.distinct) ==> always true { return vm != _latestValue; ==> always false } return true; }
  4. I'm not quite understanding
brianegan commented 1 year ago

Thanks.

Can you please put debugger or a print statement in the _whereDistinct and _handleChange methods? Could you also keep your print statement inside your widget builder function.

Print the value of vm != _latestValue in _whereDistinct and at the top of _handleChange.

The stream should not continue to _handleChange nor the builder if _whereDistinct returns false.

If you see something in the print logs like "_whereDistinct: false, _handleChange: false, builder: $vm" then something is wrong with the stream and we need to debug why.

The stream should should not trigger a widget rebuild when _whereDistinct returns false.

On Thu, Mar 16, 2023 at 04:52 Bar-Ilan Kimbarovski @.***> wrote:

  1. widget isn't rebuild outside the builder, only inside store connector builder.
  2. == method return true.
  3. bool _whereDistinct(ViewModel vm) { if (widget.distinct) ==> always true { return vm != _latestValue; ==> always false }

return true;

}

  1. I'm not quite understanding

— Reply to this email directly, view it on GitHub https://github.com/brianegan/flutter_redux/issues/242#issuecomment-1471814771, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA65DHEW6LDX2E6IOV3CHLW4L5JLANCNFSM6AAAAAAVB6LZCE . You are receiving this because you were mentioned.Message ID: @.***>

barilki commented 1 year ago

I/flutter ( 9789): _whereDistinct: false I/flutter ( 9789): _whereDistinct: true I/flutter ( 9789): _handleChange: true I/flutter ( 9789): _whereDistinct: false I/flutter ( 9789): _whereDistinct: false I/flutter ( 9789): builder: Notification Page VM

this is what it get.

brianegan commented 1 year ago

Thanks. This is what I'd expect if the Stream is working as expected. If two view models are equal using the == method, _whereDistinct correctly returns false and the Stream does not continue / the widget is not rebuilt.

However, from the pasted log, you can see that in some cases, your == method shows that two view models are not equal, so _whereDistinct returns true. This will cause the widget to rebuild, since it detects a new viewmodel has been emitted.

Please examine your == method when it returns false to figure out the root cause of the problem. In order to prevent the widget from rebuilding, _whereDistinct must return false.

barilki commented 1 year ago

allInstruments return false inside == operator

brianegan commented 1 year ago

In that case, you've found the root of the issue! The allInstruments instance variable content changes.

If the list content changes, the two view models are no longer equal, and flutter_redux will rebuild the widget tree using the builder function, passing in new view model.

To prevent a whole page from rebuilding, the best thing to do would be to remove the allInstruments variable from your view model, and wrap only the specific part of the page that needs to rebuild when allInstruments changes with a StoreConnector widget that extracts that info.