diff --git a/.github/workflows/format.yml b/.github/workflows/format.yml index 0b41ba2f392d..e77a8cfd80d6 100644 --- a/.github/workflows/format.yml +++ b/.github/workflows/format.yml @@ -1,7 +1,7 @@ name: Format on: - pull_request: + push: branch: 'master' jobs: diff --git a/Examples/CaseStudies/SwiftUICaseStudies/03-Navigation-Lists-LoadThenNavigate.swift b/Examples/CaseStudies/SwiftUICaseStudies/03-Navigation-Lists-LoadThenNavigate.swift index 7090ad4c79ea..35b73db26eb9 100644 --- a/Examples/CaseStudies/SwiftUICaseStudies/03-Navigation-Lists-LoadThenNavigate.swift +++ b/Examples/CaseStudies/SwiftUICaseStudies/03-Navigation-Lists-LoadThenNavigate.swift @@ -52,15 +52,15 @@ let lazyListNavigationReducer = Reducer< case .setNavigation(selection: .none): if let selection = state.selection { - state.rows[selection.id]?.count = selection.count + state.rows[id: selection.id]?.count = selection.count state.selection = nil } return .none case let .setNavigationSelectionDelayCompleted(id): - state.rows[id]?.isActivityIndicatorVisible = false + state.rows[id: id]?.isActivityIndicatorVisible = false state.selection = Identified( - CounterState(count: state.rows[id]?.count ?? 0), + CounterState(count: state.rows[id: id]?.count ?? 0), id: id ) return .none diff --git a/Examples/CaseStudies/SwiftUICaseStudies/03-Navigation-Lists-NavigateAndLoad.swift b/Examples/CaseStudies/SwiftUICaseStudies/03-Navigation-Lists-NavigateAndLoad.swift index 3d0993c7e80f..6590a095a76e 100644 --- a/Examples/CaseStudies/SwiftUICaseStudies/03-Navigation-Lists-NavigateAndLoad.swift +++ b/Examples/CaseStudies/SwiftUICaseStudies/03-Navigation-Lists-NavigateAndLoad.swift @@ -49,14 +49,14 @@ let eagerListNavigationReducer = Reducer< case .setNavigation(selection: .none): if let selection = state.selection, let count = selection.value?.count { - state.rows[selection.id]?.count = count + state.rows[id: selection.id]?.count = count state.selection = nil } return .cancel(id: CancelId()) case .setNavigationSelectionDelayCompleted: guard let id = state.selection?.id else { return .none } - state.selection?.value = CounterState(count: state.rows[id]?.count ?? 0) + state.selection?.value = CounterState(count: state.rows[id: id]?.count ?? 0) return .none } }, diff --git a/Sources/ComposableArchitecture/Reducer.swift b/Sources/ComposableArchitecture/Reducer.swift index b44115ad8f0a..905a73b40d55 100644 --- a/Sources/ComposableArchitecture/Reducer.swift +++ b/Sources/ComposableArchitecture/Reducer.swift @@ -194,6 +194,17 @@ public struct Reducer { guard let (index, localAction) = toLocalAction.extract(from: globalAction) else { return .none } + // NB: This does not need to be a fatal error because of the index subscript that follows it. + assert( + index < globalState[keyPath: toLocalState].endIndex, + """ + Index out of range. This can happen when a reducer that can remove the last element from \ + an array is then combined with a "forEach" from that array. To avoid this and other \ + index-related gotchas, consider using an "IdentifiedArray" of state instead. Or, combine \ + your reducers so that the "forEach" comes before any reducer that can remove elements from \ + its array. + """ + ) return self.reducer( &globalState[keyPath: toLocalState][index], localAction, @@ -238,7 +249,7 @@ public struct Reducer { guard let (id, localAction) = toLocalAction.extract(from: globalAction) else { return .none } return self.optional .reducer( - &globalState[keyPath: toLocalState][id], + &globalState[keyPath: toLocalState][id: id], localAction, toLocalEnvironment(globalEnvironment) ) diff --git a/Sources/ComposableArchitecture/SwiftUI/ForEachStore.swift b/Sources/ComposableArchitecture/SwiftUI/ForEachStore.swift index 91d8ff21042e..442ff0cca071 100644 --- a/Sources/ComposableArchitecture/SwiftUI/ForEachStore.swift +++ b/Sources/ComposableArchitecture/SwiftUI/ForEachStore.swift @@ -105,7 +105,7 @@ where Data: Collection, ID: Hashable, Content: View { ForEach(viewStore.state, id: viewStore.id) { element in content( store.scope( - state: { $0[element[keyPath: viewStore.id]] ?? element }, + state: { $0[id: element[keyPath: viewStore.id]] ?? element }, action: { (element[keyPath: viewStore.id], $0) } ) ) diff --git a/Sources/ComposableArchitecture/SwiftUI/IdentifiedArray.swift b/Sources/ComposableArchitecture/SwiftUI/IdentifiedArray.swift index 31438105de4a..5e193b33bde8 100644 --- a/Sources/ComposableArchitecture/SwiftUI/IdentifiedArray.swift +++ b/Sources/ComposableArchitecture/SwiftUI/IdentifiedArray.swift @@ -92,12 +92,15 @@ where ID: Hashable { } } - public subscript(id: ID) -> Element? { + public subscript(id id: ID) -> Element? { get { self.dictionary[id] } _modify { yield &self.dictionary[id] + if self.dictionary[id] == nil { + self.ids.removeAll(where: { $0 == id }) + } } } @@ -115,6 +118,7 @@ where ID: Hashable { } } + @discardableResult public mutating func remove(at position: Int) -> Element { let id = self.ids.remove(at: position) let element = self.dictionary[id]! @@ -138,7 +142,7 @@ where ID: Hashable { } public mutating func remove(atOffsets offsets: IndexSet) { - for offset in offsets { + for offset in offsets.reversed() { _ = self.remove(at: offset) } } diff --git a/Tests/ComposableArchitectureTests/IdentifiedArrayTests.swift b/Tests/ComposableArchitectureTests/IdentifiedArrayTests.swift new file mode 100644 index 000000000000..cf0085c9fb0c --- /dev/null +++ b/Tests/ComposableArchitectureTests/IdentifiedArrayTests.swift @@ -0,0 +1,113 @@ +import XCTest + +@testable import ComposableArchitecture + +final class IdentifiedArrayTests: XCTestCase { + func testIdSubscript() { + struct User: Equatable, Identifiable { + let id: Int + var name: String + } + + var array: IdentifiedArray = [User(id: 1, name: "Blob")] + + XCTAssertEqual(array[id: 1], .some(User(id: 1, name: "Blob"))) + + array[id: 1] = nil + XCTAssertEqual(array, []) + } + + func testInsert() { + struct User: Equatable, Identifiable { + let id: Int + var name: String + } + + var array: IdentifiedArray = [User(id: 1, name: "Blob")] + + array.insert(User(id: 2, name: "Blob Jr."), at: 0) + XCTAssertEqual(array, [User(id: 2, name: "Blob Jr."), User(id: 1, name: "Blob")]) + } + + func testInsertContentsOf() { + struct User: Equatable, Identifiable { + let id: Int + var name: String + } + + var array: IdentifiedArray = [User(id: 1, name: "Blob")] + + array.insert(contentsOf: [User(id: 3, name: "Blob Sr."), User(id: 2, name: "Blob Jr.")], at: 0) + XCTAssertEqual( + array, + [User(id: 3, name: "Blob Sr."), User(id: 2, name: "Blob Jr."), User(id: 1, name: "Blob")] + ) + } + + func testRemoveAt() { + struct User: Equatable, Identifiable { + let id: Int + var name: String + } + + var array: IdentifiedArray = [ + User(id: 3, name: "Blob Sr."), + User(id: 2, name: "Blob Jr."), + User(id: 1, name: "Blob"), + ] + + array.remove(at: 1) + XCTAssertEqual(array, [User(id: 3, name: "Blob Sr."), User(id: 1, name: "Blob")]) + } + + func testRemoveAllWhere() { + struct User: Equatable, Identifiable { + let id: Int + var name: String + } + + var array: IdentifiedArray = [ + User(id: 3, name: "Blob Sr."), + User(id: 2, name: "Blob Jr."), + User(id: 1, name: "Blob"), + ] + + array.removeAll(where: { $0.name.starts(with: "Blob ") }) + XCTAssertEqual(array, [User(id: 1, name: "Blob")]) + } + + func testRemoveAtOffsets() { + struct User: Equatable, Identifiable { + let id: Int + var name: String + } + + var array: IdentifiedArray = [ + User(id: 3, name: "Blob Sr."), + User(id: 2, name: "Blob Jr."), + User(id: 1, name: "Blob"), + ] + + array.remove(atOffsets: [0, 2]) + XCTAssertEqual(array, [User(id: 2, name: "Blob Jr.")]) + } + + func testMoveFromOffsets() { + struct User: Equatable, Identifiable { + let id: Int + var name: String + } + + var array: IdentifiedArray = [ + User(id: 3, name: "Blob Sr."), + User(id: 2, name: "Blob Jr."), + User(id: 1, name: "Blob"), + ] + + array.move(fromOffsets: [0], toOffset: 2) + XCTAssertEqual( + array, + [User(id: 2, name: "Blob Jr."), User(id: 3, name: "Blob Sr."), User(id: 1, name: "Blob")] + ) + } +}