From 12268edde87a6905c28ae305620b50c5f6a1de3f Mon Sep 17 00:00:00 2001 From: Stephen Celis Date: Mon, 11 Nov 2024 16:47:51 -0800 Subject: [PATCH] Prefer KVO for app storage observation (#3487) * wip * wip * wip --- .github/workflows/ci.yml | 43 +++++++++--- Makefile | 14 +++- .../PersistenceKey/AppStorageKey.swift | 67 ++++++++++++++----- .../PersistenceKey/AppStorageKeyPathKey.swift | 16 ----- 4 files changed, 96 insertions(+), 44 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9c2e4634be20..8c33f9dc56fa 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -14,14 +14,44 @@ concurrency: cancel-in-progress: true jobs: + xcodebuild-latest: + name: xcodebuild (16) + runs-on: macos-15 + strategy: + matrix: + command: [''] + platform: [IOS, MACOS] + xcode: ['16.0'] + steps: + - uses: actions/checkout@v4 + - name: Select Xcode ${{ matrix.xcode }} + run: sudo xcode-select -s /Applications/Xcode_${{ matrix.xcode }}.app + - name: List available devices + run: xcrun simctl list devices available + - name: Cache derived data + uses: actions/cache@v3 + with: + path: | + ~/.derivedData + key: | + deriveddata-xcodebuild-${{ matrix.platform }}-${{ matrix.xcode }}-${{ matrix.command }}-${{ hashFiles('**/Sources/**/*.swift', '**/Tests/**/*.swift') }} + restore-keys: | + deriveddata-xcodebuild-${{ matrix.platform }}-${{ matrix.xcode }}-${{ matrix.command }}- + - name: Set IgnoreFileSystemDeviceInodeChanges flag + run: defaults write com.apple.dt.XCBuild IgnoreFileSystemDeviceInodeChanges -bool YES + - name: Update mtime for incremental builds + uses: chetan/git-restore-mtime-action@v2 + - name: Debug + run: make XCODEBUILD_ARGUMENT="${{ matrix.command }}" CONFIG=Debug PLATFORM="${{ matrix.platform }}" WORKSPACE=.github/package.xcworkspace xcodebuild + xcodebuild: - name: xcodebuild + name: xcodebuild (15) runs-on: macos-14 strategy: matrix: command: [test, ''] platform: [IOS, MAC_CATALYST, MACOS, TVOS, VISIONOS, WATCHOS] - xcode: [15.2, 15.4, '16.0'] + xcode: [15.2, 15.4] exclude: - {xcode: 15.2, command: test} - {xcode: 15.4, command: ''} @@ -29,11 +59,6 @@ jobs: - {xcode: 15.2, platform: TVOS} - {xcode: 15.2, platform: VISIONOS} - {xcode: 15.2, platform: WATCHOS} - - {xcode: '16.0', command: ''} - - {xcode: '16.0', platform: MAC_CATALYST} - - {xcode: '16.0', platform: TVOS} - - {xcode: '16.0', platform: VISIONOS} - - {xcode: '16.0', platform: WATCHOS} include: - {xcode: 15.2, skip_release: 1} steps: @@ -80,7 +105,7 @@ jobs: examples: name: Examples - runs-on: macos-14 + runs-on: macos-15 steps: - uses: actions/checkout@v4 - name: Cache derived data @@ -113,7 +138,7 @@ jobs: run: make DERIVED_DATA_PATH=~/.derivedData SCHEME="Todos" xcodebuild-raw - name: VoiceMemos run: make DERIVED_DATA_PATH=~/.derivedData SCHEME="VoiceMemos" xcodebuild-raw - + check-macro-compatibility: name: Check Macro Compatibility runs-on: macos-latest diff --git a/Makefile b/Makefile index 657d0a9b76f4..aed9d7bf9a03 100644 --- a/Makefile +++ b/Makefile @@ -12,6 +12,8 @@ PLATFORM_WATCHOS = watchOS Simulator,id=$(call udid_for,watchOS,Watch) PLATFORM = IOS DESTINATION = platform="$(PLATFORM_$(PLATFORM))" +PLATFORM_ID = $(shell echo "$(DESTINATION)" | sed -E "s/.+,id=(.+)/\1/") + SCHEME = ComposableArchitecture WORKSPACE = ComposableArchitecture.xcworkspace @@ -36,11 +38,17 @@ endif TEST_RUNNER_CI = $(CI) -xcodebuild: +warm-simulator: + @test "$(PLATFORM_ID)" != "" \ + && xcrun simctl boot $(PLATFORM_ID) \ + && open -a Simulator --args -CurrentDeviceUDID $(PLATFORM_ID) \ + || exit 0 + +xcodebuild: warm-simulator $(XCODEBUILD) # Workaround for debugging Swift Testing tests: https://github.com/cpisciotta/xcbeautify/issues/313 -xcodebuild-raw: +xcodebuild-raw: warm-simulator $(XCODEBUILD_COMMAND) build-for-library-evolution: @@ -58,7 +66,7 @@ format: -not -path '*/.*' -print0 \ | xargs -0 swift format --ignore-unparsable-files --in-place -.PHONY: build-for-library-evolution format xcodebuild +.PHONY: build-for-library-evolution format warm-simulator xcodebuild xcodebuild-raw define udid_for $(shell xcrun simctl list devices available '$(1)' | grep '$(2)' | sort -r | head -1 | awk -F '[()]' '{ print $$(NF-3) }') diff --git a/Sources/ComposableArchitecture/SharedState/PersistenceKey/AppStorageKey.swift b/Sources/ComposableArchitecture/SharedState/PersistenceKey/AppStorageKey.swift index 6f8a5eddb7ea..5c78f16861c1 100644 --- a/Sources/ComposableArchitecture/SharedState/PersistenceKey/AppStorageKey.swift +++ b/Sources/ComposableArchitecture/SharedState/PersistenceKey/AppStorageKey.swift @@ -290,22 +290,41 @@ extension AppStorageKey: PersistenceKey { didSet: @escaping @Sendable (_ newValue: Value?) -> Void ) -> Shared.Subscription { let previousValue = LockIsolated(initialValue) - let userDefaultsDidChange = NotificationCenter.default.addObserver( - forName: UserDefaults.didChangeNotification, - object: self.store.wrappedValue, - queue: nil - ) { _ in - let newValue = load(initialValue: initialValue) - defer { previousValue.withValue { $0 = newValue } } - guard - !(_isEqual(newValue as Any, previousValue.value as Any) ?? false) - || (_isEqual(newValue as Any, initialValue as Any) ?? true) - else { - return + let removeObserver: () -> Void + if key.hasPrefix("@") || key.contains(".") { + let userDefaultsDidChange = NotificationCenter.default.addObserver( + forName: UserDefaults.didChangeNotification, + object: store.wrappedValue, + queue: .main + ) { _ in + let newValue = load(initialValue: initialValue) + defer { previousValue.withValue { $0 = newValue } } + func isEqual(_ lhs: T, _ rhs: T) -> Bool? { + func open(_ lhs: U) -> Bool { + lhs == rhs as? U + } + guard let lhs = lhs as? any Equatable else { return nil } + return open(lhs) + } + guard + !(isEqual(newValue, previousValue.value) ?? false) + || (isEqual(newValue, initialValue) ?? true) + else { + return + } + guard !SharedAppStorageLocals.isSetting + else { return } + didSet(newValue) } - guard !SharedAppStorageLocals.isSetting - else { return } - didSet(newValue) + removeObserver = { NotificationCenter.default.removeObserver(userDefaultsDidChange) } + } else { + let observer = Observer { + guard !SharedAppStorageLocals.isSetting + else { return } + didSet(load(initialValue: initialValue)) + } + store.wrappedValue.addObserver(observer, forKeyPath: key, options: .new, context: nil) + removeObserver = { store.wrappedValue.removeObserver(observer, forKeyPath: key) } } let willEnterForeground: (any NSObjectProtocol)? if let willEnterForegroundNotificationName { @@ -320,12 +339,28 @@ extension AppStorageKey: PersistenceKey { willEnterForeground = nil } return Shared.Subscription { - NotificationCenter.default.removeObserver(userDefaultsDidChange) + removeObserver() if let willEnterForeground { NotificationCenter.default.removeObserver(willEnterForeground) } } } + + private class Observer: NSObject { + let didChange: () -> Void + init(didChange: @escaping () -> Void) { + self.didChange = didChange + super.init() + } + override func observeValue( + forKeyPath keyPath: String?, + of object: Any?, + change: [NSKeyValueChangeKey: Any]?, + context: UnsafeMutableRawPointer? + ) { + self.didChange() + } + } } private struct AppStorageKeyID: Hashable { diff --git a/Sources/ComposableArchitecture/SharedState/PersistenceKey/AppStorageKeyPathKey.swift b/Sources/ComposableArchitecture/SharedState/PersistenceKey/AppStorageKeyPathKey.swift index 5a14e0aefbc9..84dc174992ed 100644 --- a/Sources/ComposableArchitecture/SharedState/PersistenceKey/AppStorageKeyPathKey.swift +++ b/Sources/ComposableArchitecture/SharedState/PersistenceKey/AppStorageKeyPathKey.swift @@ -60,22 +60,6 @@ extension AppStorageKeyPathKey: PersistenceKey, Hashable { observer.invalidate() } } - - private class Observer: NSObject { - let didChange: (Value?) -> Void - init(didChange: @escaping (Value?) -> Void) { - self.didChange = didChange - super.init() - } - override func observeValue( - forKeyPath keyPath: String?, - of object: Any?, - change: [NSKeyValueChangeKey: Any]?, - context: UnsafeMutableRawPointer? - ) { - self.didChange(change?[.newKey] as? Value) - } - } } // NB: This is mainly used for tests, where observer notifications can bleed across cases.