From 7072c99a241e22f04c947705312df433a0c8719a Mon Sep 17 00:00:00 2001 From: Pascal Welsch Date: Sun, 3 Jul 2022 01:57:45 +0200 Subject: [PATCH] Keeps the SyncEngine alive when the Wiredash widget updates The SyncEngine (or better their jobs) depends on the WiredashApi. The WiredashApi depends on the Wiredash widget. When the Wiredash widget changes -because the secret or the projectId have might have changed - the WiredashApi gets recreated and so does the SyncEngine. Unfortunately, this cancels the inital sync where the engine waits 5s before firing the first jobs. This fix makes sure the SyncEngine does not get recreated. It uses the same instance as long as Wiredash lives. Why wasn't this caught with tests? Well it was tested. But in tests there is no dependency from the MockWireadshApi to the Wiredash Widget, thus not causing a rebuild. Sigh... --- lib/src/core/services/services.dart | 58 ++++++++-------- lib/src/core/services/streampod.dart | 87 ++++++++++++++++-------- lib/src/core/sync/ping_job.dart | 6 +- lib/src/core/sync/sync_feedback_job.dart | 8 +-- test/common/services/streampod_test.dart | 74 +++++++++++++++----- test/sync/ping_job_test.dart | 14 ++-- test/sync/sync_engine_test.dart | 27 ++++++++ test/util/robot.dart | 4 +- test/wiredash_widget_test.dart | 38 ++++++++++- 9 files changed, 223 insertions(+), 93 deletions(-) diff --git a/lib/src/core/services/services.dart b/lib/src/core/services/services.dart index 66f662b2..8a1e649c 100644 --- a/lib/src/core/services/services.dart +++ b/lib/src/core/services/services.dart @@ -38,39 +38,39 @@ class WiredashServices extends ChangeNotifier { final Locator _locator = Locator(); - WiredashModel get wiredashModel => _locator.get(); + WiredashModel get wiredashModel => _locator.watch(); - FeedbackModel get feedbackModel => _locator.get(); + FeedbackModel get feedbackModel => _locator.watch(); - NpsModel get npsModel => _locator.get(); + NpsModel get npsModel => _locator.watch(); - BackdropController get backdropController => _locator.get(); + BackdropController get backdropController => _locator.watch(); - DeviceInfoGenerator get deviceInfoGenerator => _locator.get(); + DeviceInfoGenerator get deviceInfoGenerator => _locator.watch(); - PicassoController get picassoController => _locator.get(); + PicassoController get picassoController => _locator.watch(); - ScreenCaptureController get screenCaptureController => _locator.get(); + ScreenCaptureController get screenCaptureController => _locator.watch(); - BuildInfoManager get buildInfoManager => _locator.get(); + BuildInfoManager get buildInfoManager => _locator.watch(); - FeedbackSubmitter get feedbackSubmitter => _locator.get(); + FeedbackSubmitter get feedbackSubmitter => _locator.watch(); - DeviceIdGenerator get deviceIdGenerator => _locator.get(); + DeviceIdGenerator get deviceIdGenerator => _locator.watch(); - Wiredash get wiredashWidget => _locator.get(); + Wiredash get wiredashWidget => _locator.watch(); - WiredashOptionsData get wiredashOptions => _locator.get(); + WiredashOptionsData get wiredashOptions => _locator.watch(); - WiredashApi get api => _locator.get(); + WiredashApi get api => _locator.watch(); - SyncEngine get syncEngine => _locator.get(); + SyncEngine get syncEngine => _locator.watch(); - DiscardFeedbackUseCase get discardFeedback => _locator.get(); + DiscardFeedbackUseCase get discardFeedback => _locator.watch(); - DiscardNpsUseCase get discardNps => _locator.get(); + DiscardNpsUseCase get discardNps => _locator.watch(); - ProjectCredentialValidator get projectCredentialValidator => _locator.get(); + ProjectCredentialValidator get projectCredentialValidator => _locator.watch(); void updateWidget(Wiredash wiredashWidget) { inject((_) => wiredashWidget); @@ -83,15 +83,11 @@ class WiredashServices extends ChangeNotifier { } InstanceFactory inject( - T Function(WiredashServices) create, { - T Function(WiredashServices, T oldInstance)? update, + T Function(Locator) create, { Function(T)? dispose, }) { final factory = _locator.injectProvider( - (locator) => create(this), - update: update == null - ? null - : (_, T oldInstance) => update(this, oldInstance), + (locator) => create(_locator), dispose: dispose, ); notifyListeners(); @@ -124,7 +120,7 @@ void _setupServices(WiredashServices sl) { ); sl.inject((_) => DeviceInfoGenerator(window)); sl.inject( - (locator) => locator.wiredashWidget.options ?? const WiredashOptionsData(), + (_) => sl.wiredashWidget.options ?? const WiredashOptionsData(), ); sl.inject( (locator) => ScreenCaptureController(), @@ -150,9 +146,9 @@ void _setupServices(WiredashServices sl) { (locator) { return WiredashApi( httpClient: Client(), - projectId: locator.wiredashWidget.projectId, - secret: locator.wiredashWidget.secret, - deviceIdProvider: locator.deviceIdGenerator.deviceId, + projectId: sl.wiredashWidget.projectId, + secret: sl.wiredashWidget.secret, + deviceIdProvider: sl.deviceIdGenerator.deviceId, ); }, ); @@ -160,7 +156,7 @@ void _setupServices(WiredashServices sl) { sl.inject( (locator) { if (kIsWeb) { - return DirectFeedbackSubmitter(locator.api); + return DirectFeedbackSubmitter(sl.api); } const fileSystem = LocalFileSystem(); @@ -172,7 +168,7 @@ void _setupServices(WiredashServices sl) { uuidV4Generator: const UuidV4Generator(), ); final retryingFeedbackSubmitter = - RetryingFeedbackSubmitter(fileSystem, storage, locator.api); + RetryingFeedbackSubmitter(fileSystem, storage, sl.api); return retryingFeedbackSubmitter; }, ); @@ -184,14 +180,14 @@ void _setupServices(WiredashServices sl) { engine.addJob( 'ping', PingJob( - api: locator.api, + apiProvider: locator.get, sharedPreferencesProvider: SharedPreferences.getInstance, ), ); engine.addJob( 'feedback', UploadPendingFeedbackJob( - feedbackSubmitter: locator.feedbackSubmitter, + feedbackSubmitterProvider: locator.get, ), ); diff --git a/lib/src/core/services/streampod.dart b/lib/src/core/services/streampod.dart index 577cb39a..1a1480e8 100644 --- a/lib/src/core/services/streampod.dart +++ b/lib/src/core/services/streampod.dart @@ -20,19 +20,38 @@ class Locator { throw Exception('Locator is disposed'); } final provider = _registry[T]; - return provider!.instance as T; + return provider!.get as T; + } + + /// Retrieve a instance of type [T] + T watch() { + if (_disposed) { + throw Exception('Locator is disposed'); + } + final provider = _registry[T]; + return provider!.watch as T; + } + + /// Listen to change of type [T] + void listen(void Function(T) callback) { + final factory = _registry[T]!; + + factory.listeners.add(() { + final currentFactory = _registry[T]!; + callback(currentFactory.get as T); + }); + callback(factory.get as T); } InstanceFactory injectProvider( T Function(Locator) create, { - T Function(Locator, T oldInstance)? update, void Function(T)? dispose, }) { if (_disposed) { throw Exception('Locator is disposed'); } late InstanceFactory provider; - provider = InstanceFactory(this, create, update, () { + provider = InstanceFactory(this, create, () { final instance = provider._instance; if (instance != null && dispose != null) { dispose(instance); @@ -41,12 +60,20 @@ class Locator { final existing = _registry[T]; _registry[T] = provider; if (existing != null) { + final consumers = existing.consumers.toList(); final listeners = existing.listeners.toList(); - existing.listeners = []; + existing.consumers = []; existing.dependencies = []; + existing.listeners = []; existing.dispose?.call(); + for (final consumer in consumers) { + // rebuilding automatically registers dependencies and listeners again + consumer.rebuild(); + } + + provider.listeners.addAll(listeners); for (final listener in listeners) { - listener.rebuild(); + listener.call(); } } return provider; @@ -59,64 +86,68 @@ class InstanceFactory { InstanceFactory( this.locator, this.create, - this.update, this.dispose, ); final T Function(Locator) create; - final T Function(Locator, T oldInstance)? update; final Locator locator; final int id = _id++; T? _instance; - T? _oldInstance; - List listeners = []; + /// Those factories that use the instance and rebuild when it changes + List consumers = []; + + /// Receive notifications about instance changes + List listeners = []; + + /// When those change, instance gets recreated List dependencies = []; final Function()? dispose; late final DependencyTracker _tracker = DependencyTracker(this); - T get instance { + /// Rebuilds the calling [InstanceFactory] when [_instance] changes. + T get watch { if (_instance == null) { _tracker.create(); late final T i; - if (_oldInstance != null && update != null) { - // ignore: null_check_on_nullable_type_parameter - i = update!.call(locator, _oldInstance!); - } else { - i = create(locator); - } + i = create(locator); _tracker.created(); - // print("$T\n" - // "\t-depends on: ${dependencies.map((e) => e.runtimeType)}\n" - // "\t-Listeners: ${listeners.map((e) => e.runtimeType)}\n"); _instance = i; } else { _tracker.create(); _tracker.created(); } - return _instance!; + return _instance as T; } + /// Reads the current [_instance], doesn't subscribe the calling [InstanceFactory] to changes. + T get get { + if (_instance == null) { + final T i = create(locator); + _instance = i; + } + return _instance as T; + } + + /// Discards the current [_instance] and rebuilds it with updated dependencies. + /// + /// Tracks dependencies to other providers during rebuild void rebuild() { dispose?.call(); - final listeners = this.listeners.toList(); + final consumers = this.consumers.toList(); dependencies = []; - this.listeners = []; - _oldInstance = _instance; + this.consumers = []; _instance = null; // invalidate existing dependency instances - for (final provider in listeners) { + for (final provider in consumers) { _tracker.create(); provider.rebuild(); _tracker.created(); } - - // create instance - // instance; } @override @@ -144,7 +175,7 @@ class DependencyTracker { if (_prevActive != null) { final listener = locator._registry.values .firstWhere((element) => element.id == _prevActive); - provider.listeners.add(listener); + provider.consumers.add(listener); listener.dependencies.add(provider); } } diff --git a/lib/src/core/sync/ping_job.dart b/lib/src/core/sync/ping_job.dart index 53f5e3a7..af086ba5 100644 --- a/lib/src/core/sync/ping_job.dart +++ b/lib/src/core/sync/ping_job.dart @@ -4,11 +4,11 @@ import 'package:wiredash/src/core/network/wiredash_api.dart'; import 'package:wiredash/src/core/sync/sync_engine.dart'; class PingJob extends Job { - final WiredashApi api; + final WiredashApi Function() apiProvider; final Future Function() sharedPreferencesProvider; PingJob({ - required this.api, + required this.apiProvider, required this.sharedPreferencesProvider, }); @@ -45,7 +45,7 @@ class PingJob extends Job { } try { - await api.ping(); + await apiProvider().ping(); await _saveLastSuccessfulPing(now); syncDebugPrint('ping'); } on KillSwitchException catch (_) { diff --git a/lib/src/core/sync/sync_feedback_job.dart b/lib/src/core/sync/sync_feedback_job.dart index c762b2aa..628c4804 100644 --- a/lib/src/core/sync/sync_feedback_job.dart +++ b/lib/src/core/sync/sync_feedback_job.dart @@ -3,10 +3,10 @@ import 'package:wiredash/src/core/sync/sync_engine.dart'; import 'package:wiredash/src/feedback/_feedback.dart'; class UploadPendingFeedbackJob extends Job { - final FeedbackSubmitter feedbackSubmitter; + final FeedbackSubmitter Function() feedbackSubmitterProvider; UploadPendingFeedbackJob({ - required this.feedbackSubmitter, + required this.feedbackSubmitterProvider, }); @override @@ -16,11 +16,11 @@ class UploadPendingFeedbackJob extends Job { @override Future execute() async { - if (feedbackSubmitter is! RetryingFeedbackSubmitter) { + final submitter = feedbackSubmitterProvider(); + if (submitter is! RetryingFeedbackSubmitter) { return; } - final submitter = feedbackSubmitter as RetryingFeedbackSubmitter; await submitter.submitPendingFeedbackItems(); if (kDebugMode) { diff --git a/test/common/services/streampod_test.dart b/test/common/services/streampod_test.dart index 9c9e7634..b8441271 100644 --- a/test/common/services/streampod_test.dart +++ b/test/common/services/streampod_test.dart @@ -6,46 +6,84 @@ void main() { final sl = Locator(); final apiKeyAProvider = sl.injectProvider<_ApiKey>((_) => _ApiKey('a')); final apiProvider = - sl.injectProvider<_Api>((locator) => _Api(locator.get())); - expect(sl.get<_Api>().key.value, 'a'); + sl.injectProvider<_Api>((locator) => _Api(locator.watch())); + expect(sl.watch<_Api>().key.value, 'a'); expect(apiProvider.dependencies, [apiKeyAProvider]); - expect(apiKeyAProvider.listeners, [apiProvider]); + expect(apiKeyAProvider.consumers, [apiProvider]); final apiKeyBProvider = sl.injectProvider<_ApiKey>((_) => _ApiKey('b')); - expect(sl.get<_Api>().key.value, 'b'); + expect(sl.watch<_Api>().key.value, 'b'); expect(apiProvider.dependencies, [apiKeyBProvider]); - expect(apiKeyAProvider.listeners, []); - expect(apiKeyBProvider.listeners, [apiProvider]); + expect(apiKeyAProvider.consumers, []); + expect(apiKeyBProvider.consumers, [apiProvider]); + }); + + test('provider update when dependencies change', () { + final sl = Locator(); + final apiKeyAProvider = sl.injectProvider<_ApiKey>((_) => _ApiKey('a')); + final listenerValues = []; + final apiProvider = sl.injectProvider<_Api>( + (locator) { + final api = _Api(_ApiKey('x')); + + sl.listen<_ApiKey>((key) { + listenerValues.add(key.value); + api.key = key; + }); + + return api; + }, + ); + final aApi = sl.watch<_Api>(); + expect(aApi.key.value, 'a'); + expect(apiProvider.dependencies, []); + expect(apiKeyAProvider.consumers, []); + expect(apiKeyAProvider.listeners.length, 1); + expect(listenerValues, ['a']); + + final apiKeyBProvider = sl.injectProvider<_ApiKey>((_) => _ApiKey('b')); + + final bApi = sl.watch<_Api>(); + expect(bApi.key.value, 'b'); + expect(apiProvider.dependencies, []); + expect(apiKeyAProvider.consumers, []); + expect(apiKeyBProvider.consumers, []); + expect(apiKeyAProvider.listeners.length, 0); + expect(apiKeyBProvider.listeners.length, 1); + expect(listenerValues, ['a', 'b']); + + // same instance, because update was used + expect(bApi, same(aApi)); }); test('multi level rebuild', () { final sl = Locator(); final keyProviderA = sl.injectProvider<_ApiKey>((_) => _ApiKey('a')); final apiProvider = - sl.injectProvider<_Api>((locator) => _Api(locator.get())); + sl.injectProvider<_Api>((locator) => _Api(locator.watch())); final repoProvider = sl.injectProvider<_Repo>((locator) { - return _Repo(locator.get()); + return _Repo(locator.watch()); }); - expect(sl.get<_Repo>().apiKey, 'a'); + expect(sl.watch<_Repo>().apiKey, 'a'); expect(repoProvider.dependencies, [apiProvider]); - expect(repoProvider.listeners, []); + expect(repoProvider.consumers, []); expect(apiProvider.dependencies, [keyProviderA]); - expect(apiProvider.listeners, [repoProvider]); + expect(apiProvider.consumers, [repoProvider]); expect(keyProviderA.dependencies, []); - expect(keyProviderA.listeners, [apiProvider]); + expect(keyProviderA.consumers, [apiProvider]); final keyProviderB = sl.injectProvider<_ApiKey>((_) => _ApiKey('b')); - expect(sl.get<_Repo>().apiKey, 'b'); + expect(sl.watch<_Repo>().apiKey, 'b'); expect(repoProvider.dependencies, [apiProvider]); - expect(repoProvider.listeners, []); + expect(repoProvider.consumers, []); expect(apiProvider.dependencies, [keyProviderB]); - expect(apiProvider.listeners, [repoProvider]); + expect(apiProvider.consumers, [repoProvider]); expect(keyProviderB.dependencies, []); - expect(keyProviderB.listeners, [apiProvider]); + expect(keyProviderB.consumers, [apiProvider]); // the old one was disposed expect(keyProviderA.dependencies, []); - expect(keyProviderA.listeners, []); + expect(keyProviderA.consumers, []); }); } @@ -56,7 +94,7 @@ class _ApiKey { } class _Api { - final _ApiKey key; + _ApiKey key; _Api(this.key); } diff --git a/test/sync/ping_job_test.dart b/test/sync/ping_job_test.dart index 22958474..46cc51b1 100644 --- a/test/sync/ping_job_test.dart +++ b/test/sync/ping_job_test.dart @@ -20,7 +20,7 @@ void main() { Future prefsProvider() async => prefs; PingJob createPingJob() => PingJob( - api: api, + apiProvider: () => api, sharedPreferencesProvider: prefsProvider, ); @@ -92,8 +92,10 @@ void main() { api.pingInvocations.interceptor = (invocation) { throw const KillSwitchException(); }; - final pingJob = - PingJob(api: api, sharedPreferencesProvider: prefsProvider); + final pingJob = PingJob( + apiProvider: () => api, + sharedPreferencesProvider: prefsProvider, + ); pingJob.execute(); async.flushTimers(); expect(api.pingInvocations.count, 1); @@ -110,8 +112,10 @@ void main() { api.pingInvocations.interceptor = (invocation) { throw const SocketException('message'); }; - final pingJob = - PingJob(api: api, sharedPreferencesProvider: prefsProvider); + final pingJob = PingJob( + apiProvider: () => api, + sharedPreferencesProvider: prefsProvider, + ); pingJob.execute(); async.flushTimers(); expect(api.pingInvocations.count, 1); diff --git a/test/sync/sync_engine_test.dart b/test/sync/sync_engine_test.dart index e5007da4..f7c24c8e 100644 --- a/test/sync/sync_engine_test.dart +++ b/test/sync/sync_engine_test.dart @@ -2,8 +2,15 @@ import 'dart:async'; import 'package:clock/clock.dart'; import 'package:fake_async/fake_async.dart'; +import 'package:flutter/widgets.dart'; import 'package:test/test.dart'; +import 'package:wiredash/src/core/network/wiredash_api.dart'; +import 'package:wiredash/src/core/services/services.dart'; import 'package:wiredash/src/core/sync/sync_engine.dart'; +import 'package:wiredash/src/feedback/_feedback.dart'; +import 'package:wiredash/wiredash.dart'; + +import '../util/mock_api.dart'; void main() { group('sync engine', () { @@ -57,6 +64,26 @@ void main() { // did not update, was not executed again expect(lastExecution, firstRun); }); + + test('rebuilding SyncEngine keeps the instance', () async { + final services = WiredashServices(); + final firstSyncEngine = services.syncEngine; + // SyncEngine uses the api and submitter. Changing any of those does not + // create a new instance. + services.inject( + (_) => const Wiredash( + projectId: 'newId', + secret: 'newSecret', + child: SizedBox(), + ), + ); + services.inject((_) => MockWiredashApi()); + services.inject( + (sl) => DirectFeedbackSubmitter(sl.watch()), + ); + final secondSyncEngine = services.syncEngine; + expect(firstSyncEngine, same(secondSyncEngine)); + }); }); } diff --git a/test/util/robot.dart b/test/util/robot.dart index 2bb69575..62b479c9 100644 --- a/test/util/robot.dart +++ b/test/util/robot.dart @@ -71,7 +71,7 @@ class WiredashTestRobot { // replace submitter, because for testing we always want to submit directly robot.services.inject( - (locator) => DirectFeedbackSubmitter(locator.api), + (locator) => DirectFeedbackSubmitter(robot.services.api), ); return robot; @@ -328,7 +328,7 @@ class WiredashTestRobot { WiredashServices createMockServices() { final services = WiredashServices(); - services.inject((locator) => MockWiredashApi()); + services.inject((_) => MockWiredashApi()); return services; } diff --git a/test/wiredash_widget_test.dart b/test/wiredash_widget_test.dart index 433b6b41..b77b2451 100644 --- a/test/wiredash_widget_test.dart +++ b/test/wiredash_widget_test.dart @@ -31,6 +31,39 @@ void main() { expect(find.byType(Wiredash), findsOneWidget); }); + testWidgets('ping is send when the Widget gets updated', (tester) async { + await tester.pumpWidget( + const Wiredash( + projectId: 'test', + secret: 'invalid-secret', + // this widget never settles, allowing us to jump in the future + child: CircularProgressIndicator(), + ), + ); + final api1 = findWireadshServices.api as MockWiredashApi; + await tester.pump(); + await tester.pump(); + await tester.pump(const Duration(seconds: 1)); + await tester.pump(); + await tester.pump(); + + expect(api1.pingInvocations.count, 0); + await tester.pumpWidget( + const Wiredash( + projectId: 'test', + secret: 'correct-secret', + child: CircularProgressIndicator(), + ), + ); + await tester.pump(); + await tester.pump(); + + expect(api1.pingInvocations.count, 0); + print("wait 5s"); + await tester.pump(const Duration(seconds: 5)); + expect(api1.pingInvocations.count, 1); + }); + testWidgets('readding Wiredash simply works and sends pings again', (tester) async { await tester.pumpWidget( @@ -57,7 +90,8 @@ void main() { await tester.pumpWidget( const Wiredash( projectId: 'test', - secret: 'test', + // new secret makes the api, thus the SyncEngine rebuild + secret: 'new secret', child: SizedBox(), ), ); @@ -76,7 +110,7 @@ void main() { _MockProjectCredentialValidator(); debugServicesCreator = () => createMockServices() - ..inject((p0) => validator); + ..inject((_) => validator); addTearDown(() => debugServicesCreator = null); await tester.pumpWidget(