From a5d1b900c0a2c31c56b90b1077fff12bf03284a3 Mon Sep 17 00:00:00 2001 From: Thomas Connell Date: Thu, 8 Jun 2017 11:31:28 -0600 Subject: [PATCH 1/5] Do not subscribe to stores that are disposed or disposing. :hear_no_evil: --- lib/src/component_declaration/flux_component.dart | 10 ++++++++-- .../component_declaration/flux_component_test.dart | 12 ++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/lib/src/component_declaration/flux_component.dart b/lib/src/component_declaration/flux_component.dart index 09d1c938a..bf810d8b7 100644 --- a/lib/src/component_declaration/flux_component.dart +++ b/lib/src/component_declaration/flux_component.dart @@ -15,6 +15,7 @@ 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'; @@ -88,6 +89,9 @@ abstract class _FluxComponentMixin implements Batche /// /// These subscriptions are canceled when the component is unmounted. List _subscriptions = []; + @visibleForTesting + List get subscriptions =>_subscriptions; + void componentWillMount() { /// Subscribe to all applicable stores. @@ -101,8 +105,10 @@ abstract class _FluxComponentMixin implements Batche value: (_) => (_) => redraw())..addAll(getStoreHandlers()); handlers.forEach((store, handler) { - StreamSubscription subscription = store.listen(handler); - _subscriptions.add(subscription); + if(!store.isDisposedOrDisposing) { + StreamSubscription subscription = store.listen(handler); + _subscriptions.add(subscription); + } }); } diff --git a/test/over_react/component_declaration/flux_component_test.dart b/test/over_react/component_declaration/flux_component_test.dart index 1b42efc77..bd59f6b5b 100644 --- a/test/over_react/component_declaration/flux_component_test.dart +++ b/test/over_react/component_declaration/flux_component_test.dart @@ -65,6 +65,18 @@ void main() { reason: 'component should no longer be listening after unmount'); }); + test('does not subscribe to a store that is disposed or disposing', () async { + var store = new TestStore(); + var renderedInstance = render(TestDefault()..store = store); + TestDefaultComponent component = getDartComponent(renderedInstance); + expect(component.subscriptions.length, 1); + + await store.dispose(); + renderedInstance = render(TestDefault()..store = store); + component = getDartComponent(renderedInstance); + expect(component.subscriptions.length, 0); + }); + test('subscribes to any stores returned in redrawOn', () async { var stores = new TestStores(); var renderedInstance = render(TestRedrawOn()..store = stores); From 05bbe8c562984b7a7f62b98b2efdb0912a86a309 Mon Sep 17 00:00:00 2001 From: Thomas Connell Date: Fri, 9 Jun 2017 16:01:46 -0600 Subject: [PATCH 2/5] Just assert and log, instead. :warning: --- .../component_declaration/flux_component.dart | 19 ++++++++++++------- .../flux_component_test.dart | 12 ------------ 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/lib/src/component_declaration/flux_component.dart b/lib/src/component_declaration/flux_component.dart index bf810d8b7..a837f2cfb 100644 --- a/lib/src/component_declaration/flux_component.dart +++ b/lib/src/component_declaration/flux_component.dart @@ -15,6 +15,7 @@ library over_react.component_declaration.flux_component; import 'dart:async'; +import 'package:logging/logging.dart'; import 'package:meta/meta.dart'; import 'package:w_flux/w_flux.dart'; import './annotations.dart' as annotations; @@ -83,15 +84,13 @@ abstract class FluxUiStatefulComponent implements BatchedRedraws { + final Logger _logger = new Logger('_FluxComponentMixin'); TProps get props; /// List of store subscriptions created when the component mounts. /// /// These subscriptions are canceled when the component is unmounted. List _subscriptions = []; - @visibleForTesting - List get subscriptions =>_subscriptions; - void componentWillMount() { /// Subscribe to all applicable stores. @@ -105,10 +104,16 @@ abstract class _FluxComponentMixin implements Batche value: (_) => (_) => redraw())..addAll(getStoreHandlers()); handlers.forEach((store, handler) { - if(!store.isDisposedOrDisposing) { - StreamSubscription subscription = store.listen(handler); - _subscriptions.add(subscription); - } + String message = 'Cannot listen to a disposed/disposing Store.'; + assert(!store.isDisposedOrDisposing, '$message This can be caused by BatchedRedraws ' + 'mounting the component asynchronously after the store has been disposed. If you are ' + 'in a test environment, try adding an `await window.animationFrame;` before disposing your ' + 'store.'); + + if(store.isDisposedOrDisposing) _logger.warning(message); + + StreamSubscription subscription = store.listen(handler); + _subscriptions.add(subscription); }); } diff --git a/test/over_react/component_declaration/flux_component_test.dart b/test/over_react/component_declaration/flux_component_test.dart index bd59f6b5b..1b42efc77 100644 --- a/test/over_react/component_declaration/flux_component_test.dart +++ b/test/over_react/component_declaration/flux_component_test.dart @@ -65,18 +65,6 @@ void main() { reason: 'component should no longer be listening after unmount'); }); - test('does not subscribe to a store that is disposed or disposing', () async { - var store = new TestStore(); - var renderedInstance = render(TestDefault()..store = store); - TestDefaultComponent component = getDartComponent(renderedInstance); - expect(component.subscriptions.length, 1); - - await store.dispose(); - renderedInstance = render(TestDefault()..store = store); - component = getDartComponent(renderedInstance); - expect(component.subscriptions.length, 0); - }); - test('subscribes to any stores returned in redrawOn', () async { var stores = new TestStores(); var renderedInstance = render(TestRedrawOn()..store = stores); From 7afb657b91063073401738853b903dfd46c73170 Mon Sep 17 00:00:00 2001 From: Thomas Connell Date: Fri, 9 Jun 2017 16:02:43 -0600 Subject: [PATCH 3/5] Clean up. :wind_chime: --- lib/src/component_declaration/flux_component.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/src/component_declaration/flux_component.dart b/lib/src/component_declaration/flux_component.dart index a837f2cfb..f1b54e661 100644 --- a/lib/src/component_declaration/flux_component.dart +++ b/lib/src/component_declaration/flux_component.dart @@ -16,7 +16,6 @@ library over_react.component_declaration.flux_component; import 'dart:async'; import 'package:logging/logging.dart'; -import 'package:meta/meta.dart'; import 'package:w_flux/w_flux.dart'; import './annotations.dart' as annotations; import './transformer_helpers.dart'; From 6a4041b1178ecb7ebbf15a6416212e0d6561d958 Mon Sep 17 00:00:00 2001 From: Thomas Connell Date: Wed, 14 Jun 2017 08:47:56 -0600 Subject: [PATCH 4/5] Fix CI and CR. :heavy_check_mark: --- .travis.yml | 2 +- lib/src/component_declaration/flux_component.dart | 2 +- pubspec.yaml | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 6b54daa39..b5a5b0f29 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,6 +1,6 @@ language: dart dart: - - "1.21.1" + - "1.22.0" with_content_shell: true before_install: - export DISPLAY=:99.0 diff --git a/lib/src/component_declaration/flux_component.dart b/lib/src/component_declaration/flux_component.dart index f1b54e661..5a4c1daeb 100644 --- a/lib/src/component_declaration/flux_component.dart +++ b/lib/src/component_declaration/flux_component.dart @@ -109,7 +109,7 @@ abstract class _FluxComponentMixin implements Batche 'in a test environment, try adding an `await window.animationFrame;` before disposing your ' 'store.'); - if(store.isDisposedOrDisposing) _logger.warning(message); + if (store.isDisposedOrDisposing) _logger.warning(message); StreamSubscription subscription = store.listen(handler); _subscriptions.add(subscription); diff --git a/pubspec.yaml b/pubspec.yaml index 260b57c69..821642a1d 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -5,11 +5,12 @@ homepage: https://github.com/Workiva/over_react/ authors: - Workiva UI Platform Team environment: - sdk: ">=1.21.1" + sdk: ">=1.22.0" dependencies: analyzer: ">=0.26.1+3 <0.31.0" barback: "^0.15.0" js: "^0.6.0" + logging: ">=0.11.3+1 <1.0.0" meta: "^1.0.4" path: "^1.4.1" react: "^3.1.0" From 1c5f5a23efa50fc205a3ac8c0789f76db9c1a5f3 Mon Sep 17 00:00:00 2001 From: Thomas Connell Date: Wed, 14 Jun 2017 10:07:13 -0600 Subject: [PATCH 5/5] Make the logger static, avoid potential for memory leaks. --- lib/src/component_declaration/flux_component.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/component_declaration/flux_component.dart b/lib/src/component_declaration/flux_component.dart index 5a4c1daeb..f334caec2 100644 --- a/lib/src/component_declaration/flux_component.dart +++ b/lib/src/component_declaration/flux_component.dart @@ -83,7 +83,7 @@ abstract class FluxUiStatefulComponent implements BatchedRedraws { - final Logger _logger = new Logger('_FluxComponentMixin'); + static final Logger _logger = new Logger('_FluxComponentMixin'); TProps get props; /// List of store subscriptions created when the component mounts.