Update TCA to latest version #228

ndurell · 2024-10-24T20:01:16Z

Description

This will update us to the latest TCA which should get us to full ios 18 and swift 6 support with strict concurrency mode.

Check List

Are you changing anything with the public API?
Have you tested this change on real device?
Are your changes backwards compatible with previous SDK Versions?
Have you added unit test coverage for your changes?
Have you verified that your changes are compatible with all the operating system version this SDK currently supports?

Manual Test Plan

Supporting Materials

ndurell · 2024-11-06T15:25:50Z

.github/workflows/swift.yml

@@ -26,9 +26,9 @@ jobs:
      - name: Select Xcode ${{ matrix.xcode }}
        run: sudo xcode-select -s /Applications/Xcode_${{ matrix.xcode }}.app
      - name: Run ${{ matrix.config }} tests
-        run: make CONFIG=${{ matrix.config }} test-library
+        run: make XCODE=${{ matrix.xcode }} CONFIG=${{ matrix.config }} test-library


Test results need to be unique.

.swiftformat

.swiftlint.yml

KlaviyoCore.podspec

ndurell · 2024-11-06T15:27:23Z

Package.swift

@@ -1,11 +1,12 @@
-// swift-tools-version: 5.6
+// swift-tools-version:5.9


We need two different version of the package to support older versions of swift.

ndurell · 2024-11-06T15:27:46Z

Package.swift

 import PackageDescription

 let package = Package(
    name: "klaviyo-swift-sdk",
-    platforms: [.iOS(.v13)],
+    platforms: [.iOS(.v15), .macOS(.v10_15)],


Might not want macOS but also maybe it will just work.

Have we ever tested / know about anyone using the klaviyo sdk in a mac os app?

So people have run the ios app on a mac but I'm not sure if that's the same thing.

I ended up taking this out. We need to do a little more work to support this.

ndurell · 2024-11-06T15:28:19Z

Package.swift

+            path: "Sources/KlaviyoSwiftExtension"),
+
+        // Vendorized Things
+        .target(


These will all be combined into one thing soon.

Sources/KlaviyoSwift/Klaviyo.swift

ndurell · 2024-11-06T15:31:40Z

Sources/KlaviyoSwift/Klaviyo.swift

@@ -62,7 +59,10 @@ public struct KlaviyoSDK {
    /// - Returns: a KlaviyoSDK instance
    @discardableResult
    public func initialize(with apiKey: String) -> KlaviyoSDK {
-        dispatchOnMainThread(action: .initialize(apiKey))
+        Task {
+            let appContextInfo = await environment.appContextInfo()


Retreiving app context requires mainactor access so this can no longer be done inside the reducer. It is now done here and passed through.

ndurell · 2024-11-06T15:34:52Z

Sources/KlaviyoSwift/StateManagement/StateManagement.swift

@@ -30,28 +31,28 @@ enum RetryInfo: Equatable {
 enum KlaviyoAction: Equatable {
    /// Sets the API key to state. If the state is already initialized then the push token is moved over to the company with the API key provided in this action.
    /// Loads the state from disk and carries over existing items from the queue. This emits `completeInitialization` at the end with the state loaded from disk.
-    case initialize(String)
+    case initialize(String, AppContextInfo)


Most actions now include app context now.

ndurell · 2024-11-06T15:36:10Z

Sources/KlaviyoSwift/StateManagement/StateManagement.swift

        if action.requiresInitialization,
           case .uninitialized = state.initalizationState {
-            environment.emitDeveloperWarning("SDK must be initialized before usage.")
+            environment.logger.error("SDK must be initialized before usage.")


This requires access to mainactor and cannot be async currently so it has been switch to a logger error instead. This is done in many places.

ndurell · 2024-11-06T15:36:38Z

Sources/KlaviyoSwift/StateManagement/StateManagement.swift

-                        await send(.setExternalId(externalId))
-                    case let .setPhoneNumber(phoneNumber):
-                        await send(.setPhoneNumber(phoneNumber))
+                    case let .event(event, appContextInfo):


Pending actions also need app context.

ndurell · 2024-11-06T15:37:04Z

Sources/KlaviyoSwift/StateManagement/StateManagement.swift

-            .merge(with: environment.appLifeCycle.lifeCycleEvents().map(\.transformToKlaviyoAction).eraseToEffect())
-            .merge(with: klaviyoSwiftEnvironment.stateChangePublisher().eraseToEffect())
+            .merge(with: .run { send in
+                for await action in await klaviyoSwiftEnvironment.lifeCyclePublisher() {


changed to async implementation from combine.

ndurell · 2024-11-06T15:37:32Z

Sources/KlaviyoSwift/StateManagement/StateManagement.swift

-                    await send(.cancelInFlightRequests)
-                }))
-
+            return Effect.cancel(id: CancelIds.request)


Cancellations work differently in the newer TCA.

ndurell · 2024-11-06T15:37:53Z

Sources/KlaviyoSwift/StateManagement/StateManagement.swift

@@ -320,7 +333,7 @@ struct KlaviyoReducer: ReducerProtocol {
                state.flushing = false
                return .none
            }
-            return .task { .sendRequest }.cancellable(id: RequestId.self)
+            return .send(.sendRequest).cancellable(id: CancelIds.request)


.task is now .send.

ndurell · 2024-11-06T15:38:47Z

Sources/KlaviyoSwiftExtension/KlaviyoExtension.swift

@@ -5,7 +5,7 @@
 //  Created by Ajay Subramanya on 6/23/23.
 //
 import Foundation
-import UserNotifications
+@preconcurrency import UserNotifications


might want a pragma here...

ndurell · 2024-11-06T15:39:27Z

Sources/KlaviyoUI/KlaviyoWebView/KlaviyoWebViewController.swift

@@ -80,7 +80,8 @@ class KlaviyoWebViewController: UIViewController, WKUIDelegate {
        Task { [weak self] in
            guard let self else { return }

-            for await (script, callback) in self.viewModel.scriptStream {
+            let scriptStream = self.viewModel.scriptStream


Caught by swift strict concurrency mode.

ndurell · 2024-11-06T15:40:36Z

Sources/KlaviyoUI/KlaviyoWebView/KlaviyoWebViewModel.swift

-    private var continuation: AsyncStream<(script: String, callback: ((Result<Any?, Error>) -> Void)?)>.Continuation?
-    lazy var scriptStream: AsyncStream<(script: String, callback: ((Result<Any?, Error>) -> Void)?)> = AsyncStream { [weak self] continuation in
+    private var continuation: AsyncStream < (script: String, callback: (@Sendable (Result<Any?, Error>) -> Void)?)>.Continuation?
+    lazy var scriptStream: AsyncStream < (script: String, callback: (@Sendable (Result<Any?, Error>) -> Void)?)> = AsyncStream { [weak self] continuation in


Needs to be Sendable.

ndurell · 2024-11-08T16:54:58Z

Sources/KlaviyoSwift/Klaviyo.swift

@@ -27,10 +24,9 @@ func dispatchOnMainThread(action: KlaviyoAction) {
 /// ```
 ///
 /// From there you can you can call the additional methods below to track events and profile.
+@MainActor


This is probably one of the more significant changes in this PR.

Sources/KlaviyoSDKDependencies/CustomDump/CustomDumpReflectable.swift

Makefile

ndurell · 2024-11-14T23:58:17Z

Can we add comments to all TCA files to indicate that they were either copied verbatim from TCA or modified from TCA, and add the TCA version number that the file was copied from? I have already run into situations where I've tried to diff our local TCA files from the ones in the TCA repo, and having the version number right in the comments have been very helpful to know exactly where the code came from.

For example, at the top of a file that was copied verbatim from TCA, we might add :
/// Copied verbatim from TCA v1.15.2 on 11/14/2024
/// https://github.com/pointfreeco/swift-composable-architecture/tree/1.15.2
And if the file has been modified from the original TCA file, we might add:
/// Adapted from TCA v1.15.2 on 11/14/2024
/// https://github.com/pointfreeco/swift-composable-architecture/tree/1.15.2
/// Changes from TCA:
/// <add brief summary of what's different between our local version and the official TCA version>

💯 great idea!

Sources/KlaviyoCore/KlaviyoEnvironment.swift

Sources/KlaviyoCore/Models/APIModels/CreateEventPayload.swift

Sources/KlaviyoCore/Models/PushBackground.swift

Sources/KlaviyoCore/KlaviyoEnvironment.swift

Sources/KlaviyoSDKDependencies/CasePaths/Internal/TypeName.swift

ab1470 · 2024-11-15T16:53:10Z

Sources/KlaviyoSDKDependencies/CasePaths/AnyCasePath.swift

with this and all other imported files, can we re-indent with 4 spaces instead of 2?

Is it better to import as they so they can be diffed easily?

Sources/KlaviyoSDKDependencies/ComposableArchitecture/CaseReducer.swift

dan-peluso · 2024-12-02T15:32:46Z

KlaviyoSDKDependencies.podspec

+  s.homepage         = "https://github.com/klaviyo/klaviyo-swift-sdk"
+  s.license          = { :type => "MIT", :file => "LICENSE" }
+  s.author           = { "Mobile @ Klaviyo" => "[email protected]" }
+  s.source           = { :git => "https://github.com/klaviyo/klaviyo-swift-sdk.git", :tag => s.version.to_s }


I'm probably not looking in the right file, but I am curious what dependencies are going in this. I saw in a lower file that both core and this were imported, why is that? Is this where the dependencies of our SDK live?

We shouldn't have any dependencies after this change in the sdk properly. I've internned pretty much everything except in tests which uses combine schedulers and a few other misc things.

dan-peluso · 2024-12-02T15:37:45Z

Sources/KlaviyoCore/AppContextInfo.swift

+private let _defaultAppContextInfo: AppContextInfo? = nil
+
+@MainActor
+public func getDefaultAppContextInfo() -> AppContextInfo {


Just want to ask about this - was this change something necessary with the TCA update, or was this just a simple enough change with the refactor? Seems like a decent sized change, going from a getter computed value rather than a struct with static values. I will admit though I am confused how something can be static but also computed at runtime with the ternary operator (although maybe its more compile time since we'll know about those info values then)

Honestly I struggled with this part a lot because we had inconsistent access to this data in the sdk and some of it required main thread access. With these changes now we can at least guarantee we always access this info on the main thread.

dan-peluso · 2024-12-02T16:29:28Z

Sources/KlaviyoCore/Models/PushEnablement.swift

@@ -7,7 +7,7 @@

 import UIKit

-public enum PushEnablement: String, Codable {
+public enum PushEnablement: String, Codable, Sendable {


Does adding all of these imply that we didn't have concurrency protection in previous versions? Or was this also necessary to add with the TCA updates.

I think actually other than the bug that was caught by an outside developer we were being pretty careful with concurrency. For a lot of these changes we are just making the compiler happy but hopefully it will stop us from making future concurrency mistakes.

dan-peluso · 2024-12-02T16:45:01Z

Sources/KlaviyoSDKDependencies/AnyCodable/AnyCodable.swift

+
+extension AnyCodable: _AnyEncodable, _AnyDecodable {}
+
+extension AnyCodable: Equatable {


very suprised this isn't natively supported in swift. Is this something that we do get with structs but not necessarily codables? Makes sense that we need it for state management operations but I'm just surprised to see it so explicit. Does this mean we need to manually add any new types supported from the backend?

See my later comment.

dan-peluso · 2024-12-02T16:49:34Z

Sources/KlaviyoSDKDependencies/AnyCodable/AnyEncodable.swift

+    }
+
+    #if canImport(Foundation)
+    private func encode(nsnumber: NSNumber, into container: inout SingleValueEncodingContainer) throws {


this whole function is wild to me

This is part of a dependency called AnyCodable. We encode some json with arbitrary key/value pairs. Anycodable helps this play nice with swift codable.

dan-peluso · 2024-12-02T16:55:02Z

Definitely this is something exacerbated by my lack of ios domain knowledge, but I'm wondering if it could be helpful to point out some of the main files / directories we should be checking out for reviewing. I'm guessing all the 'sendable added' files are probably pretty arbitrary, but are there others that are more or less copy/paste code that are not super important for review? Going to still try reviewing incrementally but feeling a smidge overwhelmed by the content still

ndurell · 2024-12-10T18:21:59Z

Definitely this is something exacerbated by my lack of ios domain knowledge, but I'm wondering if it could be helpful to point out some of the main files / directories we should be checking out for reviewing. I'm guessing all the 'sendable added' files are probably pretty arbitrary, but are there others that are more or less copy/paste code that are not super important for review? Going to still try reviewing incrementally but feeling a smidge overwhelmed by the content still

@dan-peluso I put a header on any file that was taken from tca or other repos. That should be a good indicator it was copied verbatim. The header has information on whether the content was modified and what changed. Credit to @ab1470 for the idea!

* Update TCA to latest * incorporate Anycodable * fix many concurrency issues

ab1470

still reviewing the overall PR but here are some minor suggestions to work on while I continue reviewing

Sources/KlaviyoSDKDependencies/IssueReporting/IssueReporter.swift

Sources/KlaviyoSDKDependencies/IssueReporting/IssueReporters/RuntimeWarningReporter.swift

Tests/KlaviyoSwiftTests/Vendor/ComposableArchitecture/KeyPath+Sendable.swift

Tests/KlaviyoSwiftTests/Vendor/ComposableArchitecture/DispatchQueue.swift

Tests/KlaviyoSwiftTests/Vendor/CustomDump/Conformances/CoreLocation.swift

Tests/KlaviyoSwiftTests/Vendor/CustomDump/Conformances/Foundation.swift

Tests/KlaviyoSwiftTests/Vendor/CustomDump/Conformances/KeyPath.swift

Tests/KlaviyoSwiftTests/Vendor/CustomDump/Conformances/Swift.swift

Tests/KlaviyoSwiftTests/Vendor/IdentifiedCollections/Collections.swift

remove `unchecked` from `FileClient`

ab1470

😮‍💨 big one. Left a bunch of comments but mostly minor stuff. I'll look into the KlaviyoWebView/ViewModel tomorrow to see what I can do to fix it.

ab1470 · 2024-12-18T21:25:49Z

Sources/KlaviyoSDKDependencies/IssueReporting/IssueReporters/RuntimeWarningReporter.swift

@@ -25,8 +25,6 @@ extension IssueReporter where Self == _RuntimeWarningReporter {
 /// fault-level messages to the console.
 ///
 /// Use ``IssueReporter/runtimeWarning`` to create one of these values.
-/// Copied verbatim from xctest dynamic overlay v1.3.0 on 11/15/2024
-/// https://github.com/pointfreeco/xctest-dynamic-overlay/tree/1.3.0



Suggested change

Nit: remove a space to make sure that inline documentation works properly

ab1470 · 2024-12-19T16:18:15Z

Sources/KlaviyoCore/KlaviyoEnvironment.swift

+                    }
+                }
+            }
+        }, appContextInfo: {


Suggested change

}, appContextInfo: {

},

appContextInfo: {

nit, aligning parameter names

ab1470 · 2024-12-19T18:48:20Z

Sources/KlaviyoCore/KlaviyoEnvironment.swift

 }

 public enum KlaviyoDecodingError: Error {
    case invalidType
    case invalidJson
 }

-public struct DataDecoder {
+public struct DataDecoder: @unchecked Sendable {


I tried removing the @unchecked to see what would happen, and I didn't see any compiler errors or warnings. Are you sure we need it?

note, I was testing on Xcode 16.2. Maybe there are warnings/errors on earlier versions?

ab1470 · 2024-12-19T18:56:40Z

Sources/KlaviyoCore/Models/APIModels/CreateEventPayload.swift

-    fileprivate func appendMetadataToProperties(pushToken: String?) -> [String: Any]? {
-        let context = environment.appContextInfo()
-        let metadata: [String: Any] = [
+    func appendMetadataToProperties(context: AppContextInfo, pushToken: String?) -> [String: Any]? {


Suggested change

func appendMetadataToProperties(context: AppContextInfo, pushToken: String?) -> [String: Any]? {

fileprivate func appendMetadataToProperties(context: AppContextInfo, pushToken: String?) -> [String: Any]? {

Was there a reason for removing the fileprivate? I tried re-adding it and it seems fine. No compiler issues

ab1470 · 2024-12-19T19:44:22Z

Sources/KlaviyoCore/KlaviyoEnvironment.swift

+        stopTimer()
+
+        // Create a new DispatchSourceTimer and start it
+        let newTimer = DispatchSource.makeTimerSource(queue: .global())


Suggested change

let newTimer = DispatchSource.makeTimerSource(queue: .global())

let timerQueue = DispatchQueue(label: "com.klaviyo.TimerActor.timerQueue")

let newTimer = DispatchSource.makeTimerSource(queue: timerQueue)

ChatGPT made this suggestion, so big grain of salt here. Its explanation: "Currently, the DispatchSourceTimer is created on a global queue. While functional, this approach does not take advantage of the actor’s natural thread confinement, which ensures thread safety. You can improve it by creating the timer on a custom serial queue for better control."

I searched Github and it seems like a lot of people create timers on the global queue or the main queue, but some create a custom queue. I'm not sure what the right answer is, maybe it doesn't really matter? What do you think?

ab1470 · 2024-12-19T22:13:26Z

Sources/KlaviyoSwift/KlaviyoSwiftEnvironment.swift

+            },
+            getBackgroundSetting: {
+                .create(from: UIApplication.shared.backgroundRefreshStatus)
+            }, networkSession: { createNetworkSession() },


Suggested change

}, networkSession: { createNetworkSession() },

},

networkSession: { createNetworkSession() },

nit, aligning all parameters

ab1470 · 2024-12-19T22:46:50Z

Tests/KlaviyoCoreTests/TestUtils.swift

 extension LoggerClient {
    static var lastLoggedMessage: String?
    static let test = LoggerClient { message in
        lastLoggedMessage = message
    }
 }

+@MainActor


Suggested change

@MainActor

looks like this isn't necessary

ab1470 · 2024-12-19T22:47:10Z

Tests/KlaviyoCoreTests/TestUtils.swift

 }

+@MainActor


Suggested change

@MainActor

same here

ab1470 · 2024-12-19T22:49:01Z

Tests/KlaviyoSwiftTests/AppLifeCycleEventsTests.swift

 @testable import KlaviyoSwift
-import Combine
+@preconcurrency import Combine // Will figure out a better way for this...


did you intend to leave this comment in?

ab1470 · 2024-12-19T23:47:50Z

Sources/KlaviyoUI/KlaviyoWebView/KlaviyoWebViewModel.swift

+            Task {
+                let script = await WKUserScript(source: closeHandlerScript, injectionTime: .atDocumentEnd, forMainFrameOnly: true)
+                scripts["closeHandler"] = script
+            }


hmm, by dispatching this to a task, the KlaviyoWebView will read the loadScripts property before the closeHandler script has been loaded. I can think some more tomorrow about how to resolve this; it might involve some refactoring of the View/ViewModel

ndurell force-pushed the noah/update_tca branch from d94679a to 47015e3 Compare November 5, 2024 20:59

klaviyo deleted a comment from nightfall-for-github bot Nov 5, 2024

ndurell marked this pull request as ready for review November 6, 2024 15:14

ndurell requested a review from a team as a code owner November 6, 2024 15:14

ndurell requested a review from jasper-hsu-kyvo November 6, 2024 15:14

ndurell changed the title ~~[DNM] - Update TCA to latest version~~ Update TCA to latest version Nov 6, 2024

ndurell requested a review from ajaysubra November 6, 2024 15:15