Skip to content

Commit 56b1c19

Browse files
bsneedbsneed
authored
Ensure update(settings:type:) is only called with .initial once. (#276)
1 parent 7f4e9e7 commit 56b1c19

File tree

6 files changed

+119
-24
lines changed

6 files changed

+119
-24
lines changed

‎Sources/Segment/Plugins.swift

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,6 @@ extension Plugin {
9393
}
9494
}
9595

96-
9796
// MARK: - Adding/Removing Plugins
9897

9998
extension DestinationPlugin {
@@ -127,6 +126,7 @@ extension DestinationPlugin {
127126
plugin.configure(analytics: analytics)
128127
}
129128
timeline.add(plugin: plugin)
129+
analytics?.updateIfNecessary(plugin: plugin)
130130
return plugin
131131
}
132132

@@ -189,6 +189,7 @@ extension Analytics {
189189
public func add(plugin: Plugin) -> Plugin {
190190
plugin.configure(analytics: self)
191191
timeline.add(plugin: plugin)
192+
updateIfNecessary(plugin: plugin)
192193
return plugin
193194
}
194195

‎Sources/Segment/Settings.swift

Lines changed: 36 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -108,20 +108,44 @@ extension Settings: Equatable {
108108
}
109109

110110
extension Analytics {
111-
internal func update(settings: Settings, type: UpdateType) {
112-
apply { (plugin) in
113-
// tell all top level plugins to update.
114-
update(plugin: plugin, settings: settings, type: type)
111+
internal func update(settings: Settings) {
112+
guard let system: System = store.currentState() else { return }
113+
apply { plugin in
114+
plugin.update(settings: settings, type: updateType(for: plugin, in: system))
115+
if let destPlugin = plugin as? DestinationPlugin {
116+
destPlugin.apply { subPlugin in
117+
subPlugin.update(settings: settings, type: updateType(for: subPlugin, in: system))
118+
}
119+
}
115120
}
116121
}
117122

118-
internal func update(plugin: Plugin, settings: Settings, type: UpdateType) {
119-
plugin.update(settings: settings, type: type)
120-
// if it's a destination, tell it's plugins to update as well.
121-
if let dest = plugin as? DestinationPlugin {
122-
dest.apply { (subPlugin) in
123-
subPlugin.update(settings: settings, type: type)
123+
internal func updateIfNecessary(plugin: Plugin) {
124+
guard let system: System = store.currentState() else { return }
125+
// if we're already running, update has already been called for existing plugins,
126+
// so we just wanna call it on this one if it hasn't been done already.
127+
if system.running, let settings = system.settings {
128+
let alreadyInitialized = system.initializedPlugins.contains { p in
129+
return plugin === p
124130
}
131+
if !alreadyInitialized {
132+
store.dispatch(action: System.AddPluginToInitialized(plugin: plugin))
133+
plugin.update(settings: settings, type: .initial)
134+
} else {
135+
plugin.update(settings: settings, type: .refresh)
136+
}
137+
}
138+
}
139+
140+
internal func updateType(for plugin: Plugin, in system: System) -> UpdateType {
141+
let alreadyInitialized = system.initializedPlugins.contains { p in
142+
return plugin === p
143+
}
144+
if alreadyInitialized {
145+
return .refresh
146+
} else {
147+
store.dispatch(action: System.AddPluginToInitialized(plugin: plugin))
148+
return .initial
125149
}
126150
}
127151

@@ -135,6 +159,7 @@ extension Analytics {
135159
operatingMode.run(queue: DispatchQueue.main) {
136160
if let state: System = self.store.currentState(), let settings = state.settings {
137161
self.store.dispatch(action: System.UpdateSettingsAction(settings: settings))
162+
self.update(settings: settings)
138163
}
139164
self.store.dispatch(action: System.ToggleRunningAction(running: true))
140165
}
@@ -145,9 +170,6 @@ extension Analytics {
145170

146171
let writeKey = self.configuration.values.writeKey
147172
let httpClient = HTTPClient(analytics: self)
148-
let systemState: System? = store.currentState()
149-
let hasSettings = (systemState?.settings?.integrations != nil && systemState?.settings?.plan != nil)
150-
let updateType = (hasSettings ? UpdateType.refresh : UpdateType.initial)
151173

152174
// stop things; queue in case our settings have changed.
153175
store.dispatch(action: System.ToggleRunningAction(running: false))
@@ -158,7 +180,7 @@ extension Analytics {
158180
// this will cause them to be cached.
159181
self.store.dispatch(action: System.UpdateSettingsAction(settings: s))
160182
// let plugins know we just received some settings..
161-
self.update(settings: s, type: updateType)
183+
self.update(settings: s)
162184
}
163185
}
164186
// we're good to go back to a running state.

‎Sources/Segment/State.swift

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ struct System: State {
1515
let settings: Settings?
1616
let running: Bool
1717
let enabled: Bool
18+
let initializedPlugins: [Plugin]
1819

1920
struct UpdateSettingsAction: Action {
2021
let settings: Settings
@@ -23,7 +24,8 @@ struct System: State {
2324
let result = System(configuration: state.configuration,
2425
settings: settings,
2526
running: state.running,
26-
enabled: state.enabled)
27+
enabled: state.enabled,
28+
initializedPlugins: state.initializedPlugins)
2729
return result
2830
}
2931
}
@@ -35,7 +37,8 @@ struct System: State {
3537
return System(configuration: state.configuration,
3638
settings: state.settings,
3739
running: running,
38-
enabled: state.enabled)
40+
enabled: state.enabled,
41+
initializedPlugins: state.initializedPlugins)
3942
}
4043
}
4144

@@ -46,7 +49,8 @@ struct System: State {
4649
return System(configuration: state.configuration,
4750
settings: state.settings,
4851
running: state.running,
49-
enabled: enabled)
52+
enabled: enabled,
53+
initializedPlugins: state.initializedPlugins)
5054
}
5155
}
5256

@@ -57,7 +61,8 @@ struct System: State {
5761
return System(configuration: configuration,
5862
settings: state.settings,
5963
running: state.running,
60-
enabled: state.enabled)
64+
enabled: state.enabled,
65+
initializedPlugins: state.initializedPlugins)
6166
}
6267
}
6368

@@ -73,7 +78,26 @@ struct System: State {
7378
return System(configuration: state.configuration,
7479
settings: settings,
7580
running: state.running,
76-
enabled: state.enabled)
81+
enabled: state.enabled,
82+
initializedPlugins: state.initializedPlugins)
83+
}
84+
}
85+
86+
struct AddPluginToInitialized: Action {
87+
let plugin: Plugin
88+
89+
func reduce(state: System) -> System {
90+
var initializedPlugins = state.initializedPlugins
91+
if !initializedPlugins.contains(where: { p in
92+
return plugin === p
93+
}) {
94+
initializedPlugins.append(plugin)
95+
}
96+
return System(configuration: state.configuration,
97+
settings: state.settings,
98+
running: state.running,
99+
enabled: state.enabled,
100+
initializedPlugins: initializedPlugins)
77101
}
78102
}
79103
}
@@ -147,7 +171,7 @@ extension System {
147171
settings = Settings(writeKey: configuration.values.writeKey, apiHost: HTTPClient.getDefaultAPIHost())
148172
}
149173
}
150-
return System(configuration: configuration, settings: settings, running: false, enabled: true)
174+
return System(configuration: configuration, settings: settings, running: false, enabled: true, initializedPlugins: [Plugin]())
151175
}
152176
}
153177

‎Sources/Segment/Timeline.swift

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,6 @@ public class Timeline {
5555
internal class Mediator {
5656
internal func add(plugin: Plugin) {
5757
plugins.append(plugin)
58-
if let settings = plugin.analytics?.settings() {
59-
plugin.update(settings: settings, type: .initial)
60-
}
6158
}
6259

6360
internal func remove(plugin: Plugin) {

‎Tests/Segment-Tests/Analytics_Tests.swift

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,50 @@ final class Analytics_Tests: XCTestCase {
5050
wait(for: [expectation], timeout: 1.0)
5151
}
5252

53+
func testDestinationInitialUpdateOnlyOnce() {
54+
// need to clear settings for this one.
55+
UserDefaults.standard.removePersistentDomain(forName: "com.segment.storage.test")
56+
57+
let expectation = XCTestExpectation(description: "MyDestination Expectation")
58+
let myDestination = MyDestination {
59+
expectation.fulfill()
60+
return true
61+
}
62+
63+
var settings = Settings(writeKey: "test")
64+
if let existing = settings.integrations?.dictionaryValue {
65+
var newIntegrations = existing
66+
newIntegrations[myDestination.key] = true
67+
settings.integrations = try! JSON(newIntegrations)
68+
}
69+
let configuration = Configuration(writeKey: "test")
70+
configuration.defaultSettings(settings)
71+
let analytics = Analytics(configuration: configuration)
72+
73+
let ziggy1 = ZiggyPlugin()
74+
analytics.add(plugin: myDestination)
75+
analytics.add(plugin: ziggy1)
76+
77+
waitUntilStarted(analytics: analytics)
78+
79+
analytics.track(name: "testDestinationEnabled")
80+
81+
let ziggy2 = ZiggyPlugin()
82+
analytics.add(plugin: ziggy2)
83+
84+
let dest = analytics.find(key: myDestination.key)
85+
XCTAssertNotNil(dest)
86+
XCTAssertTrue(dest is MyDestination)
87+
88+
wait(for: [expectation], timeout: 1.0)
89+
90+
XCTAssertEqual(myDestination.receivedInitialUpdate, 1)
91+
XCTAssertEqual(ziggy1.receivedInitialUpdate, 1)
92+
XCTAssertEqual(ziggy2.receivedInitialUpdate, 1)
93+
94+
}
95+
96+
5397
func testDestinationEnabled() {
5498
// need to clear settings for this one.
5599
UserDefaults.standard.removePersistentDomain(forName: "com.segment.storage.test")

‎Tests/Segment-Tests/Support/TestUtilities.swift

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,13 +42,18 @@ class GooberPlugin: EventPlugin {
4242
class ZiggyPlugin: EventPlugin {
4343
let type: PluginType
4444
var analytics: Analytics?
45+
var receivedInitialUpdate: Int = 0
4546

4647
var completion: (() -> Void)?
4748

4849
required init() {
4950
self.type = .enrichment
5051
}
5152

53+
func update(settings: Settings, type: UpdateType) {
54+
if type == .initial { receivedInitialUpdate += 1 }
55+
}
56+
5257
func identify(event: IdentifyEvent) -> IdentifyEvent? {
5358
var newEvent = IdentifyEvent(existing: event)
5459
newEvent.userId = "ziggy"
@@ -78,6 +83,7 @@ class MyDestination: DestinationPlugin {
7883
let trackCompletion: (() -> Bool)?
7984

8085
let disabled: Bool
86+
var receivedInitialUpdate: Int = 0
8187

8288
init(disabled: Bool = false, trackCompletion: (() -> Bool)? = nil) {
8389
self.key = "MyDestination"
@@ -88,6 +94,7 @@ class MyDestination: DestinationPlugin {
8894
}
8995

9096
func update(settings: Settings, type: UpdateType) {
97+
if type == .initial { receivedInitialUpdate += 1 }
9198
if disabled == false {
9299
// add ourselves to the settings
93100
analytics?.manuallyEnableDestination(plugin: self)

0 commit comments

Comments
 (0)