Skip to content

Conversation

debkanchan
Copy link
Contributor

Fixes #11

No need to call createClient anymore. Instead, initialise client once using Analytics.init and continue using the plugin using Analytics.instance

Note: Testing doesn't work, mocks were broken but also fixed in this pr. Testing produces this

flutter test                                                                                                            ⏎ ✖ ✹ ✭
00:01 +2 -1: /Users/debkanchan/Code/open-source/analytics_flutter/packages/core/test/utils/http_client_test.dart: HTTP Client It logs on bad response for get Settings [E]
  Error loading storage: PlatformException(channel-error, Unable to establish connection on channel., null, null)
  package:analytics/state.dart 202:7      PersistedState.init.<fn>
  ===== asynchronous gap ===========================
  dart:async                              Future.catchError
  package:analytics/state.dart 198:8      PersistedState.init
  package:analytics/state.dart 35:13      StateManager.init
  package:analytics/analytics.dart 86:12  new Analytics.internal
  test/utils/http_client_test.dart 24:48  main.<fn>.<fn>
  
  This test failed after it had already completed.
  Make sure to use a matching library which informs the test runner
  of pending async work.
  package:analytics/state.dart 202:7      PersistedState.init.<fn>
  ===== asynchronous gap ===========================
  dart:async                              Future.catchError
  package:analytics/state.dart 198:8      PersistedState.init
  package:analytics/state.dart 35:13      StateManager.init
  package:analytics/analytics.dart 86:12  new Analytics.internal
  test/utils/http_client_test.dart 24:48  main.<fn>.<fn>
  
00:02 +7 -3: /Users/debkanchan/Code/open-source/analytics_flutter/packages/core/test/analytics_test.dart: analytics it fetches settings and fires track event when tracking lifecycle events [E]
  No matching calls. All calls: [VERIFIED] MockHTTPClient.settingsFor('123')
  (If you called `verify(...).called(0);`, please instead use `verifyNever(...);`.)
  package:matcher                        fail
  package:mockito/src/mock.dart 797:7    _VerifyCall._checkWith
  package:mockito/src/mock.dart 1071:18  _makeVerify.<fn>
  test/analytics_test.dart 57:13         main.<fn>.<fn>
  

To run this test again: /Users/debkanchan/flutter/bin/cache/dart-sdk/bin/dart test /Users/debkanchan/Code/open-source/analytics_flutter/packages/core/test/analytics_test.dart -p vm --plain-name 'analytics it fetches settings and fires track event when tracking lifecycle events'
00:05 +8 -3: Some tests failed.

@debkanchan debkanchan changed the title Use analytics as a singleton instead of global variable refactor: Use analytics as a singleton instead of global variable Jul 7, 2023
@debkanchan debkanchan changed the title refactor: Use analytics as a singleton instead of global variable refactor!: Use analytics as a singleton instead of global variable Jul 7, 2023
@debkanchan
Copy link
Contributor Author

@oscb

@debkanchan
Copy link
Contributor Author

@oscb review please?

@oscb
Copy link
Contributor

oscb commented Jul 13, 2023

@DebkanchanSamadder Thanks for all the hard work on this PR! It's very complete.

Overall at Segment's SDKs we've been trying to get away from singletons on our libraries, only offering them for compatibility. While convenient we have clients who need to use multiple clients in the same app or for testing purposes.

I'll close this but I appreciate the effort!

@oscb oscb closed this Jul 13, 2023
@debkanchan
Copy link
Contributor Author

debkanchan commented Jul 26, 2023

@oscb there's another option. We can keep this init for the default instance and also have a newInstance factory constructor to make additional instances which will return a new instance directly instead of assigning it to the instance static variable. This will ensure both ease of use and extensibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of plugin as a singleton
2 participants