-
Notifications
You must be signed in to change notification settings - Fork 15
[CHNL-29461] Add on/off switch for logging capabilities #523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
bd3edeb
da46dc7
1b32912
e384600
952fc83
7bdf920
5970121
448f991
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| // Created by Andrew Balmer on 1/28/25. | ||
| // | ||
|
|
||
| import KlaviyoCore | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing explicit dependency in Package.swift. This file now imports .target(
name: "KlaviyoForms",
dependencies: ["KlaviyoSwift"], // ← no explicit KlaviyoCore
...
)This works today because Please add .target(
name: "KlaviyoForms",
dependencies: ["KlaviyoSwift", "KlaviyoCore"],
...
)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in da46dc7 — Added "KlaviyoCore" as an explicit dependency for the KlaviyoForms target in Package.swift. |
||
| import OSLog | ||
|
|
||
| @available(iOS 14.0, *) | ||
|
|
@@ -21,13 +22,19 @@ extension Logger { | |
| @available(iOS 14.0, *) | ||
| extension Logger { | ||
| /// Logger for Javascript console log messages from a WKWebView relayed to the native layer. | ||
| static let webViewConsoleLogger = Logger(category: "WKWebView Console Log Relay") | ||
| static var webViewConsoleLogger: Logger { | ||
| KlaviyoLogConfig.shared.isLoggingEnabled ? Logger(category: "WKWebView Console Log Relay") : Logger(OSLog.disabled) | ||
| } | ||
|
|
||
| /// Logger for WKWebView related events. | ||
| /// | ||
| /// - Note: Javascript console logs relayed to the native layer should be handled by the ``webViewConsoleLogger``. | ||
| static let webViewLogger = Logger(category: "WKWebView Event Handling") | ||
| static var webViewLogger: Logger { | ||
| KlaviyoLogConfig.shared.isLoggingEnabled ? Logger(category: "WKWebView Event Handling") : Logger(OSLog.disabled) | ||
| } | ||
|
|
||
| /// Logger for filesystem operations. | ||
| static let filesystem = Logger(category: "Filesystem") | ||
| static var filesystem: Logger { | ||
| KlaviyoLogConfig.shared.isLoggingEnabled ? Logger(category: "Filesystem") : Logger(OSLog.disabled) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,12 +5,15 @@ | |
| // Created by Andrew Balmer on 10/8/24. | ||
| // | ||
|
|
||
| import KlaviyoCore | ||
| import OSLog | ||
|
|
||
| @available(iOS 14.0, *) | ||
| extension Logger { | ||
| private static var subsystem = Bundle.main.bundleIdentifier ?? "" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just now catching this -- not major but shouldn't this be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 5970121 — the KlaviyoLocation subsystem is now Generated by Claude Code |
||
|
|
||
| /// Logs events related to location services. | ||
| static let geoservices = Logger(subsystem: subsystem, category: "geoservices") | ||
| static var geoservices: Logger { | ||
| KlaviyoLogConfig.shared.isLoggingEnabled ? Logger(subsystem: subsystem, category: "geoservices") : Logger(OSLog.disabled) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| // | ||
| // KlaviyoLogConfigTests.swift | ||
| // klaviyo-swift-sdk | ||
| // | ||
|
|
||
| @testable import KlaviyoCore | ||
| import OSLog | ||
| import XCTest | ||
|
|
||
| @available(iOS 14.0, *) | ||
| final class KlaviyoLogConfigTests: XCTestCase { | ||
| override func tearDown() { | ||
| // Always restore default state after each test | ||
| KlaviyoLogConfig.shared.isLoggingEnabled = true | ||
| super.tearDown() | ||
| } | ||
|
|
||
| // MARK: - Default State | ||
|
|
||
| func testDefaultIsEnabled() { | ||
| XCTAssertTrue(KlaviyoLogConfig.shared.isLoggingEnabled) | ||
| } | ||
|
|
||
| // MARK: - Toggle Behavior | ||
|
|
||
| func testDisableLogging() { | ||
| KlaviyoLogConfig.shared.isLoggingEnabled = false | ||
| XCTAssertFalse(KlaviyoLogConfig.shared.isLoggingEnabled) | ||
| } | ||
|
|
||
| func testReEnableLogging() { | ||
| KlaviyoLogConfig.shared.isLoggingEnabled = false | ||
| XCTAssertFalse(KlaviyoLogConfig.shared.isLoggingEnabled) | ||
|
|
||
| KlaviyoLogConfig.shared.isLoggingEnabled = true | ||
| XCTAssertTrue(KlaviyoLogConfig.shared.isLoggingEnabled) | ||
| } | ||
|
|
||
| // MARK: - Logger Instances | ||
|
|
||
| func testLoggersReturnRealLoggerWhenEnabled() { | ||
| KlaviyoLogConfig.shared.isLoggingEnabled = true | ||
|
|
||
| let logger = Logger.networking | ||
| // A real logger will have a non-disabled log; verify it doesn't crash | ||
| // and that it's different from a disabled logger. | ||
| // We can't directly compare Logger instances, but we can verify the | ||
| // computed property returns without error. | ||
| _ = logger | ||
| } | ||
|
|
||
| func testLoggersReturnDisabledLoggerWhenDisabled() { | ||
| KlaviyoLogConfig.shared.isLoggingEnabled = false | ||
|
|
||
| // These should all return Logger(OSLog.disabled) without crashing | ||
| let codable = Logger.codable | ||
| let networking = Logger.networking | ||
| let navigation = Logger.navigation | ||
|
|
||
| // Verify they don't crash when called | ||
| codable.info("This should be silenced") | ||
| networking.info("This should be silenced") | ||
| navigation.info("This should be silenced") | ||
| } | ||
|
|
||
| // MARK: - Thread Safety | ||
|
|
||
| func testConcurrentAccess() { | ||
| let iterations = 1000 | ||
| let expectation = expectation(description: "Concurrent access completes") | ||
| expectation.expectedFulfillmentCount = iterations * 2 | ||
|
|
||
| let queue = DispatchQueue(label: "test.concurrent", attributes: .concurrent) | ||
|
|
||
| for _ in 0..<iterations { | ||
| queue.async { | ||
| KlaviyoLogConfig.shared.isLoggingEnabled = false | ||
| expectation.fulfill() | ||
| } | ||
| queue.async { | ||
| _ = KlaviyoLogConfig.shared.isLoggingEnabled | ||
| expectation.fulfill() | ||
| } | ||
| } | ||
|
|
||
| wait(for: [expectation], timeout: 10) | ||
| } | ||
|
|
||
| func testConcurrentToggleDoesNotCrash() { | ||
| let iterations = 500 | ||
| let expectation = expectation(description: "Concurrent toggle completes") | ||
| expectation.expectedFulfillmentCount = iterations * 2 | ||
|
|
||
| let queue = DispatchQueue(label: "test.concurrent.toggle", attributes: .concurrent) | ||
|
|
||
| for i in 0..<iterations { | ||
| queue.async { | ||
| KlaviyoLogConfig.shared.isLoggingEnabled = (i % 2 == 0) | ||
| expectation.fulfill() | ||
| } | ||
| queue.async { | ||
| // Access a logger while toggling | ||
| _ = Logger.networking | ||
| expectation.fulfill() | ||
| } | ||
| } | ||
|
|
||
| wait(for: [expectation], timeout: 10) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using
packageaccess level instead ofpublic.Since the package uses
swift-tools-version: 5.9, you can use thepackageaccess level here. This would makeKlaviyoLogConfigaccessible from all modules within the package (KlaviyoCore, KlaviyoSwift, KlaviyoForms, KlaviyoLocation) but not from external SDK consumers.Currently, with
publicaccess, SDK consumers can bypass the intendedKlaviyoSDK.setLoggingEnabled(_:)API and directly callKlaviyoLogConfig.shared.isLoggingEnabled = false. Usingpackageaccess would enforce the designed API surface:This ensures consumers use
KlaviyoSDK().setLoggingEnabled(_:)as intended.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in da46dc7 — Changed KlaviyoLogConfig from
publictopackageaccess level (class, shared instance, and isLoggingEnabled property). SDK consumers must now useKlaviyoSDK().setLoggingEnabled(_:)as the intended API surface.