UIP-2114: Update FluxUiComponent subscriptions when props are updated

lib/src/component_declaration/flux_component.dart

@@ -15,7 +15,9 @@
 library over_react.component_declaration.flux_component;
 
 import 'dart:async';
+import 'package:meta/meta.dart';
 import 'package:w_flux/w_flux.dart';
+
 import './annotations.dart' as annotations;
 import './transformer_helpers.dart';
 
@@ -64,7 +66,30 @@ abstract class FluxUiProps<ActionsT, StoresT> extends UiProps {
 ///
 /// Use with the over_react transformer via the `@Component()` ([annotations.Component]) annotation.
 abstract class FluxUiComponent<TProps extends FluxUiProps> extends UiComponent<TProps>
-    with _FluxComponentMixin<TProps>, BatchedRedraws {}
+    with _FluxComponentMixin<TProps>, BatchedRedraws {
+  // Redeclare these lifecycle methods with `mustCallSuper`, since `mustCallSuper` added to methods within
+  // mixins doesn't work. See https://github.com/dart-lang/sdk/issues/29861
+
+  @mustCallSuper
+  @override
+  // Ignore this warning to work around https://github.com/dart-lang/sdk/issues/29860
+  // ignore: must_call_super
+  void componentWillMount();
+
+  @mustCallSuper
+  @override
+  // Ignore this warning to work around https://github.com/dart-lang/sdk/issues/29860
+  // ignore: must_call_super
+  void componentWillReceiveProps(Map prevProps);
+
+  @mustCallSuper
+  @override
+  void componentDidUpdate(Map prevProps, Map prevState);
+
+  @mustCallSuper
+  @override
+  void componentWillUnmount();
+}
 
 /// Builds on top of [UiStatefulComponent], adding `w_flux` integration, much like the [FluxComponent] in w_flux.
 ///
@@ -76,46 +101,101 @@ abstract class FluxUiComponent<TProps extends FluxUiProps> extends UiComponent<T
 /// Use with the over_react transformer via the `@Component()` ([annotations.Component]) annotation.
 abstract class FluxUiStatefulComponent<TProps extends FluxUiProps, TState extends UiState>
     extends UiStatefulComponent<TProps, TState>
-    with _FluxComponentMixin<TProps>, BatchedRedraws {}
+    with _FluxComponentMixin<TProps>, BatchedRedraws {
+  // Redeclare these lifecycle methods with `mustCallSuper`, since `mustCallSuper` added to methods within
+  // mixins doesn't work. See https://github.com/dart-lang/sdk/issues/29861
+
+  @mustCallSuper
+  @override
+  // Ignore this warning to work around https://github.com/dart-lang/sdk/issues/29860
+  // ignore: must_call_super
+  void componentWillMount();
+
+  @mustCallSuper
+  @override
+  // Ignore this warning to work around https://github.com/dart-lang/sdk/issues/29860
+  // ignore: must_call_super
+  void componentWillReceiveProps(Map prevProps);
+
+  @mustCallSuper
+  @override
+  void componentDidUpdate(Map prevProps, Map prevState);
+
+  @mustCallSuper
+  @override
+  void componentWillUnmount();
+}
 
 /// Helper mixin to keep [FluxUiComponent] and [FluxUiStatefulComponent] clean/DRY.
 ///
 /// Private so it will only get used in this file, since having lifecycle methods in a mixin is risky.
-abstract class _FluxComponentMixin<TProps extends FluxUiProps> implements BatchedRedraws {
-  TProps get props;
-
+abstract class _FluxComponentMixin<TProps extends FluxUiProps> implements BatchedRedraws, UiComponent<TProps> {
   /// List of store subscriptions created when the component mounts.
   ///
   /// These subscriptions are canceled when the component is unmounted.
-  List<StreamSubscription> _subscriptions = [];
+  List<StreamSubscription> _subscriptions;
+
+  bool get _areStoreHandlersBound => _subscriptions != null;
+
+  /// Subscribe to all applicable stores.
+  ///
+  /// [Store]s returned by [redrawOn] will have their triggers mapped directly to this components
+  /// redraw function.
+  ///
+  /// [Store]s included in the [getStoreHandlers] result will be listened to and wired up to their
+  /// respective handlers.
+  void _bindStoreHandlers() {
+    if (_areStoreHandlersBound) {
+      throw new StateError('Store handlers are already bound');
+    }
 
-  void componentWillMount() {
-    /// Subscribe to all applicable stores.
-    ///
-    /// [Store]s returned by [redrawOn] will have their triggers mapped directly to this components
-    /// redraw function.
-    ///
-    /// [Store]s included in the [getStoreHandlers] result will be listened to and wired up to their
-    /// respective handlers.
     Map<Store, StoreHandler> handlers = new Map.fromIterable(redrawOn(),
         value: (_) => (_) => redraw())..addAll(getStoreHandlers());
 
+    _subscriptions = <StreamSubscription>[];
     handlers.forEach((store, handler) {
       StreamSubscription subscription = store.listen(handler);
       _subscriptions.add(subscription);
     });
   }
 
+  /// Cancel all store subscriptions.
+  void _unbindStoreHandlers() {
+    if (!_areStoreHandlersBound) return;
+
+    for (var subscription in _subscriptions) {
+      subscription?.cancel();
+    }
+
+    _subscriptions = null;
+  }
+
+  @override
+  void componentWillMount() {
+    _bindStoreHandlers();
+  }
+
+  @override
+  void componentWillReceiveProps(Map prevProps) {
+    // Unbind store handlers so they can be re-bound in componentDidUpdate
+    // once the new props are available, ensuring the values returned [redrawOn]
+    // are not outdated.
+    _unbindStoreHandlers();
+  }
+
+  @override
+  void componentDidUpdate(Map prevProps, Map prevState) {
+    // If the handlers are not bound at this point, then that means they were unbound by
+    // componentWillReceiveProps, and need to be re-bound now that new props are available.
+    if (!_areStoreHandlersBound) _bindStoreHandlers();
+  }
+
+  @override
   void componentWillUnmount() {
     // Ensure that unmounted components don't batch render
     shouldBatchRedraw = false;
 
-    // Cancel all store subscriptions.
-    _subscriptions.forEach((StreamSubscription subscription) {
-      if (subscription != null) {
-        subscription.cancel();
-      }
-    });
+    _unbindStoreHandlers();
   }
 
   /// Define the list of [Store] instances that this component should listen to.

pubspec.yaml

@@ -22,6 +22,7 @@ dev_dependencies:
   matcher: ">=0.11.0 <0.13.0"
   coverage: "^0.7.2"
   dart_dev: "^1.7.6"
+  over_react_test: "^1.0.0"
   mockito: "^0.11.0"
   test: "^0.12.6+2"
 

test/over_react/component_declaration/flux_component_test.dart

@@ -8,8 +8,7 @@ import 'dart:html';
 import 'package:test/test.dart';
 import 'package:w_flux/w_flux.dart';
 import 'package:over_react/over_react.dart';
-
-import '../../test_util/test_util.dart';
+import 'package:over_react_test/over_react_test.dart';
 
 part 'flux_component_test/default.dart';
 part 'flux_component_test/handler_precedence.dart';
@@ -164,6 +163,30 @@ void main() {
       await nextTick();
       expect(component.numberOfRedraws, equals(0));
     });
+
+    test('updates the subscriptions registered from redrawOn when rerendered with new props', () async {
+      var stores = new TestStores();
+      var newStores = new TestStores();
+
+      var testJacket = mount<TestRedrawOnComponent>((TestRedrawOn()..store = stores)());
+      var dartInstance = testJacket.getDartInstance();
+      expect(dartInstance.numberOfRedraws, 0);
+
+      stores.store1.trigger();
+      await nextTick();
+      expect(dartInstance.numberOfRedraws, 1);
+
+      testJacket.rerender((TestRedrawOn()..store = newStores)());
+      dartInstance.numberOfRedraws = 0;
+
+      newStores.store1.trigger();
+      await nextTick();
+      expect(dartInstance.numberOfRedraws, 1, reason: 'should have redrawn from new store');
+
+      stores.store1.trigger();
+      await nextTick();
+      expect(dartInstance.numberOfRedraws, 1, reason: 'should not have redrawn from old store');
+    });
   });
 }