Prefer KVO for app storage observation (#3487)

* wip

* wip

* wip

Author: stephencelis

.github/workflows/ci.yml

@@ -14,26 +14,51 @@ 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: ''}
           - {xcode: 15.2, platform: MAC_CATALYST}
           - {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

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) }')

Sources/ComposableArchitecture/SharedState/PersistenceKey/AppStorageKey.swift

@@ -290,22 +290,41 @@ extension AppStorageKey: PersistenceKey {
     didSet: @escaping @Sendable (_ newValue: Value?) -> Void
   ) -> Shared<Value>.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<T>(_ lhs: T, _ rhs: T) -> Bool? {
+          func open<U: Equatable>(_ 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 {

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.