From afaac2690cf5eccf113ffcebb93d61b1edfe36d9 Mon Sep 17 00:00:00 2001 From: Greg Littlefield Date: Wed, 1 Dec 2021 16:40:28 -0700 Subject: [PATCH 1/2] Auto-tear-down ConnectFluxAdapterStores when backing Flux stores dispose --- analysis_options.yaml | 1 + lib/over_react_flux.dart | 2 +- lib/src/over_react_redux/over_react_flux.dart | 20 +++ .../connect_flux_adapter_store_test.dart | 130 ++++++++++++++++++ test/over_react_redux_test.dart | 2 + 5 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 test/over_react_redux/connect_flux_adapter_store_test.dart diff --git a/analysis_options.yaml b/analysis_options.yaml index 9a2145442..581cdd299 100644 --- a/analysis_options.yaml +++ b/analysis_options.yaml @@ -10,6 +10,7 @@ analyzer: - tools/analyzer_plugin/** - ddc_precompiled/** errors: + invalid_export_of_internal_element: warning unused_import: warning duplicate_import: warning missing_required_param: error diff --git a/lib/over_react_flux.dart b/lib/over_react_flux.dart index c76e0fd03..2c2a90a9a 100644 --- a/lib/over_react_flux.dart +++ b/lib/over_react_flux.dart @@ -12,6 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -export 'src/over_react_redux/over_react_flux.dart'; +export 'src/over_react_redux/over_react_flux.dart' hide ConnectFluxAdapterStoreTestingHelper; export 'src/over_react_redux/redux_multi_provider.dart'; export 'src/over_react_redux/over_react_redux.dart' show ReduxProvider; diff --git a/lib/src/over_react_redux/over_react_flux.dart b/lib/src/over_react_redux/over_react_flux.dart index 069fa0d54..72f055dfd 100644 --- a/lib/src/over_react_redux/over_react_flux.dart +++ b/lib/src/over_react_redux/over_react_flux.dart @@ -131,15 +131,35 @@ class ConnectFluxAdapterStore extends redux.Store { }); actionsForStore[store] = actions; + + // This store is useless once the flux store is disposed, so for convenience, + // we'll tear it down for consumers. + // + // In most cases, though, from a memory management standpoint, tearing this + // store down shouldn't be necessary, since any components subscribed to it + // should have also been unmounted, leaving nothing to retain it. + store.didDispose.whenComplete(teardown); } + bool _teardownCalled = false; + @override Future teardown() async { + _teardownCalled = true; + await _storeListener.cancel(); await super.teardown(); } } +/// Not to be exported; only used to expose private fields for testing. +@internal +@visibleForTesting +extension ConnectFluxAdapterStoreTestingHelper on ConnectFluxAdapterStore { + @visibleForTesting + bool get teardownCalled => _teardownCalled; +} + /// Adapts a Flux store to the interface of a Redux store. /// /// When using an Influx architecture, this store allows Redux, Flux, and [connectFlux] diff --git a/test/over_react_redux/connect_flux_adapter_store_test.dart b/test/over_react_redux/connect_flux_adapter_store_test.dart new file mode 100644 index 000000000..bcbaf2a97 --- /dev/null +++ b/test/over_react_redux/connect_flux_adapter_store_test.dart @@ -0,0 +1,130 @@ +// Copyright 2021 Workiva Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import 'dart:async'; + +import 'package:over_react/src/over_react_redux/over_react_flux.dart'; +import 'package:test/test.dart'; +import 'package:w_flux/w_flux.dart' as flux; + +main() { + // See connect_flux_integration_test.dart for more test coverage of this class + group('ConnectFluxAdapterStore', () { + group('teardown', () { + test('calls super, closing change subscriptions', () async { + final adapterStore = SimpleStore().asConnectFluxStore(null); + expect(() => adapterStore.onChange.listen((_) {}), returnsNormally, + reason: 'test setup check: the stream should be open'); + + await adapterStore.teardown(); + }); + + test('stops listening to the flux store', () async { + final store = SpyStore(); + final adapterStore = store.asConnectFluxStore(null); + + expect(store.spiedSubscriptions, hasLength(1)); + // ignore: cancel_subscriptions + final sub = store.spiedSubscriptions.single; + + final subCalls = []; + sub.onData(subCalls.add); + + store.trigger(); + await pumpEventQueue(); + expect(subCalls, hasLength(1), + reason: 'test setup check; subscription should have gotten event'); + + await adapterStore.teardown(); + store.trigger(); + await pumpEventQueue(); + expect(subCalls, hasLength(1), reason: 'subscription should have been cancelled'); + }); + + group('can be called multiple times without throwing', () { + ConnectFluxAdapterStore adapterStore; + + setUp(() { + adapterStore = SimpleStore().asConnectFluxStore(null); + }); + + test('synchronously', () async { + await Future.wait([ + adapterStore.teardown(), + adapterStore.teardown(), + ]); + }); + + test('synchronously', () async { + await adapterStore.teardown(); + await adapterStore.teardown(); + }); + }); + }); + + group('calls teardown when the flux store is disposed', () { + flux.Store store; + ConnectFluxAdapterStore adapterStore; + + setUp(() { + store = SimpleStore(); + adapterStore = store.asConnectFluxStore(null); + }); + + test('', () async { + expect(adapterStore.teardownCalled, isFalse); + + await store.dispose(); + await pumpEventQueue(); + expect(adapterStore.teardownCalled, isTrue); + }); + + group('and doesn\'t break if teardown', () { + test('has already been called', () async { + expect(adapterStore.teardownCalled, isFalse); + + await adapterStore.teardown(); + expect(adapterStore.teardownCalled, isTrue); + + await store.dispose(); + await pumpEventQueue(); + }); + + test('is called after', () async { + expect(adapterStore.teardownCalled, isFalse); + + await store.dispose(); + await pumpEventQueue(); + expect(adapterStore.teardownCalled, isTrue); + + await adapterStore.teardown(); + }); + }); + }); + }); +} + +class SimpleStore extends flux.Store {} + +class SpyStore extends flux.Store { + final spiedSubscriptions = []; + + @override + listen(onData, {onError, onDone, cancelOnError}) { + final sub = + super.listen(onData, onError: onError, onDone: onDone, cancelOnError: cancelOnError); + spiedSubscriptions.add(sub); + return sub; + } +} diff --git a/test/over_react_redux_test.dart b/test/over_react_redux_test.dart index 092ca19be..2f86133cc 100644 --- a/test/over_react_redux_test.dart +++ b/test/over_react_redux_test.dart @@ -27,6 +27,7 @@ import 'over_react_redux/hooks/use_selector_test.dart' as use_selector_hook_test import 'over_react_redux/hooks/use_store_test.dart' as use_store_hook_test; import './over_react_redux/connect_test.dart' as connect_test; import './over_react_redux/connect_flux_test.dart' as connect_flux_test; +import './over_react_redux/connect_flux_adapter_store_test.dart' as connect_flux_adapter_store_test; import './over_react_redux/connect_flux_integration_test.dart' as connect_flux_integration_test; import './over_react_redux/redux_multi_provider_test.dart' as multi_provider_test; import './over_react_redux/value_mutation_checker_test.dart' as value_mutation_checker_test; @@ -39,6 +40,7 @@ void main() { use_store_hook_test.main(); connect_test.main(); connect_flux_test.main(); + connect_flux_adapter_store_test.main(); connect_flux_integration_test.main(); multi_provider_test.main(); value_mutation_checker_test.main(); From 39ceb42313d728edcb7bb4c238d416f1364e4159 Mon Sep 17 00:00:00 2001 From: Greg Littlefield Date: Wed, 1 Dec 2021 18:34:14 -0700 Subject: [PATCH 2/2] Work around mock stores having a null didDispose --- lib/src/over_react_redux/over_react_flux.dart | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/src/over_react_redux/over_react_flux.dart b/lib/src/over_react_redux/over_react_flux.dart index 72f055dfd..ce961fd3c 100644 --- a/lib/src/over_react_redux/over_react_flux.dart +++ b/lib/src/over_react_redux/over_react_flux.dart @@ -138,7 +138,9 @@ class ConnectFluxAdapterStore extends redux.Store { // In most cases, though, from a memory management standpoint, tearing this // store down shouldn't be necessary, since any components subscribed to it // should have also been unmounted, leaving nothing to retain it. - store.didDispose.whenComplete(teardown); + // + // Use a null-aware to accommodate mock stores in unit tests that return null for `didDispose`. + store.didDispose?.whenComplete(teardown); } bool _teardownCalled = false;