MBL-1016: Create Paginator for pagination in SwiftUI/Combine #1939

amy-at-kickstarter · 2024-02-08T21:08:08Z

📲 What

This adds a new class, Paginator, inspired by our existing paginate method. Paginator is designed to fill the same functionality, but in a way that integrates neatly with SwiftUI and Combine-based code.

🤔 Why

When we move over to SwiftUI and Combine, we'll need an easy way to page through long lists of API results.

🛠 How

Originally I wanted to create a 1-for-1 re-write of paginate in Combine. The nice thing about the original paginate method is that it's stateless, nothing but a bunch of pure functions. However, I spent a significant amount of time trying to duplicate it, and was unable to make any traction. Writing stateless functional code is significantly more challenging (to me at least), and I don't think the benefits of its elegance outweigh the difficult I had modifying and understanding it. Throw in a few more variables, and I think Paginator will be easier to modify and understand in the long run.

paginate came with a great deal of tests, which I used to understand its behavior, and that became my inspiration for writing Paginator.

Note that paginate includes some niche features - you can pass in a customer concater for example - that I chose not to implement. If we need those, we can implement them in the future. Most of those niche features are only used on one or two screens.

👀 See

I created PaginationExampleView and PaginationExampleViewModel as examples (and my own test bed!) for how Paginator could be used in the app. You can try it yourself by throwing in this somewhere in the app: presentViewController(UIHostingController(rootView: PaginationExampleView(), animated: true))

Pagination.example.mp4

amy-at-kickstarter · 2024-02-08T21:18:02Z

Kickstarter-iOS/Features/PaginationExample/PaginationExampleView.swift

+public struct PaginationExampleView: View {
+  @StateObject private var viewModel = PaginationExampleViewModel()
+
+  public var body: some View {


Note that I very intentionally split up PaginationExampleView from PaginationExampleProjectList. PaginationExampleProjectList is decoupled from the view model, basically containing only display logic for simple types. This made it really easy to write the SwiftUI preview; instead of having to mock out an entire environment with a view model and stubbed networking on so on, I could just pass in some strings.

amy-at-kickstarter · 2024-02-08T21:19:09Z

Kickstarter-iOS/Features/PaginationExample/PaginationExampleView.swift

+}
+
+private struct PaginationExampleProjectList: View {
+  @Binding var projectIdsAndTitles: [(Int, String)]


See my note below - the actual display view is just taking simple types. This could take a list of Projects, and that's what I did originally, but it got a little annoying - I had to use Project.template in the preview, and that's marked as private to KsApi. So I chose to just keep it to strings-and-ints for now.

amy-at-kickstarter · 2024-02-08T21:21:32Z

Library/Paginator.swift

+    self.handleRequest(request)
+  }
+
+  public func requestNextPage() {


I find this kind of declarative implementation much easier to grok than the original functional implementation of paginate.

amy-at-kickstarter · 2024-02-08T21:21:59Z

Library/PaginatorTests.swift

+  let cursorFromEnvelope: (TestEnvelope) -> Int? = { $0.cursor }
+
+  func waitTinyInterval() {
+    _ = XCTWaiter.wait(for: [expectation(description: "Wait a tiny interval of time.")], timeout: 0.05)


Easy/cheaters' method for testing asynchronous code 🙈

amy-at-kickstarter · 2024-02-08T21:23:12Z

Kickstarter-iOS/Features/PaginationExample/PaginationExampleView.swift

+      if showProgressView {
+        HStack {
+          Spacer()
+          Text("Loading 😉")


I noticed a weird bug (?) where ProgressView sometimes disappeared while scrolling. Out of scope to figure out why, so you get a winky face instead of a spinner.

nativeksr · 2024-02-08T21:25:38Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

scottkicks

Awesome work! I just have some questions that are mostly due to my lack of swiftui/combine knowledge

scottkicks · 2024-02-12T14:10:34Z

Kickstarter-iOS/Features/PaginationExample/PaginationExampleView.swift

+      Spacer()
+    }
+    .padding(EdgeInsets(top: 20, leading: 0, bottom: 20, trailing: 0))
+    .background(Color.yellow)


nit: you can remove Color and just use .yellow.

scottkicks · 2024-02-12T14:10:52Z

Kickstarter-iOS/Features/PaginationExample/PaginationExampleView.swift

+        let title = $0.1
+        PaginationExampleProjectCell(title: $0.1)


looks like we can remove this line, or use it as the title argument in PaginationExampleProjectCell

Suggested change

let title = $0.1

PaginationExampleProjectCell(title: $0.1)

PaginationExampleProjectCell(title: $0.1)

scottkicks · 2024-02-12T14:17:46Z

Kickstarter-iOS/Features/PaginationExample/PaginationExampleView.swift

+  let onRefresh: @Sendable() -> Void
+  let onDidShowProgressView: @Sendable() -> Void


@sendable allows for these methods to run concurrently, yes?

I initially put this tag here because Xcode insisted that I did (IIRC, something like these properties were implicitly @Sendable and I needed to make them explicit). I've never used @Sendable before, so I took a peek in the docs, and you are correct that it allows a function to be used "across concurrency domains." My guess is that here, pragmatically, it's enforcing something at compile time that's important - perhaps that every captured variable included in onRefresh and onDidShowProgressView won't be de-allocated? But we should probably learn more if this comes up frequently.

Huh, y'know what? It compiles fine without it. Maybe this was an artifact of an earlier version of this code. I'm going to clean this up.

scottkicks · 2024-02-12T14:33:34Z

KsApi/Service+RequestHelpers.swift

@@ -64,4 +51,35 @@ extension Service {
      .rac_dataResponse(preparedRequest(forURL: paginationUrl), and: self.perimeterXClient)
      .flatMap(self.decodeModelToSignal)
  }
+
+  func requestPaginationDecodable<M: Decodable>(_ paginationUrl: String)


What does M represent here?

M is the generic type placeholder for the model that will be decoded, for example, a Project or a Backing. The file already used M but I could rename it to Model for clarity, if it would help.

Got it. I figured! I would prefer Model here but totally up to you. Just a nit 🙂

scottkicks · 2024-02-12T14:36:51Z

Library/Paginator.swift

+
+  You can observe the results of `values`, `isLoading`, `error` and `state` to access the loaded data.
+
+ */


Awesome documentation!

I take very little credit, this is largely copy-pasted from the original paginate 🤫

scottkicks · 2024-02-12T14:39:35Z

Library/Paginator.swift

+        guard let self = self else {
+          return
+        }


nit: latest swift simplifies this guard. just fyi 😄

Suggested change

guard let self = self else {

return

}

guard let self else { return }

scottkicks · 2024-02-12T14:51:33Z

Kickstarter-iOS/Features/PaginationExample/PaginationExampleView.swift

+  @StateObject private var viewModel = PaginationExampleViewModel()
+
+  public var body: some View {
+    let capturedViewModel = viewModel


Could you explain why we need to capture the viewModel here instead of using the @StateObject var directly in PaginationExampleProjectList?

It's a side-effect of these two closures being marked @Sendable. Without the capture, you get useful, but not terribly clear error:

Main actor-isolated property 'viewModel' can not be referenced from a Sendable closure

Fixed, see note above about removing @Sendable

scottkicks

👍🏻 just some failing ci tests to work out

…ExampleView for example usage

amy-at-kickstarter changed the title ~~Feat/adyer/mbl 1016/combine for api v1/paginator~~ MBL-1016: Create Paginator for pagination in Combine Feb 8, 2024

amy-at-kickstarter changed the title ~~MBL-1016: Create Paginator for pagination in Combine~~ MBL-1016: Create Paginator for pagination in SwiftUI/Combine Feb 8, 2024

amy-at-kickstarter requested review from ifosli and scottkicks February 8, 2024 21:15

amy-at-kickstarter self-assigned this Feb 8, 2024

amy-at-kickstarter added the swiftui and combine label Feb 8, 2024

amy-at-kickstarter commented Feb 8, 2024

View reviewed changes

amy-at-kickstarter force-pushed the feat/adyer/mbl-1016/combine-for-api-v1/paginator branch from 0ee83f6 to 73b2860 Compare February 8, 2024 21:20

amy-at-kickstarter marked this pull request as ready for review February 8, 2024 21:22

amy-at-kickstarter commented Feb 8, 2024

View reviewed changes

scottkicks reviewed Feb 12, 2024

View reviewed changes

amy-at-kickstarter added a commit that referenced this pull request Feb 12, 2024

Address comments from review #1939

a4fdd05

amy-at-kickstarter requested a review from scottkicks February 12, 2024 22:23

amy-at-kickstarter added a commit that referenced this pull request Feb 13, 2024

Address comments from review #1939

cafbcba

amy-at-kickstarter force-pushed the feat/adyer/mbl-1016/combine-for-api-v1/paginator branch from 86a0803 to b8a246c Compare February 13, 2024 14:47

scottkicks approved these changes Feb 13, 2024

View reviewed changes

amy-at-kickstarter added 8 commits February 13, 2024 10:47

MBL-1016: Add requestPaginationDecodable for Combine

3f0c9b6

Add fetchDiscovery_combine, as example for pagination with Combine

b9b82de

MBL-1016: Create Paginator for pagination in Combine, plus Pagination…

e3fb9f2

…ExampleView for example usage

Address comments from review #1939

df47e8e

Remove unnecessary use of @sendable

cc3b4f0

Make PR comment into a code comment, for posterity

6bb35f9

Make PR comment into a code comment, for posterity

9d52859

Per review: rename 'M' generic type to 'Model' for clarity

d9d25ac

amy-at-kickstarter force-pushed the feat/adyer/mbl-1016/combine-for-api-v1/paginator branch from 0f87f10 to d9d25ac Compare February 13, 2024 15:47

amy-at-kickstarter merged commit 8730d16 into main Feb 13, 2024
5 checks passed

amy-at-kickstarter deleted the feat/adyer/mbl-1016/combine-for-api-v1/paginator branch February 13, 2024 16:12

amy-at-kickstarter added a commit that referenced this pull request Feb 13, 2024

Address comments from review #1939

d566c19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MBL-1016: Create Paginator for pagination in SwiftUI/Combine #1939

MBL-1016: Create Paginator for pagination in SwiftUI/Combine #1939

amy-at-kickstarter commented Feb 8, 2024 •

edited

amy-at-kickstarter Feb 8, 2024

amy-at-kickstarter Feb 8, 2024 •

edited

amy-at-kickstarter Feb 8, 2024

amy-at-kickstarter Feb 8, 2024

amy-at-kickstarter Feb 8, 2024

nativeksr commented Feb 8, 2024

scottkicks left a comment

scottkicks Feb 12, 2024

amy-at-kickstarter Feb 12, 2024

scottkicks Feb 12, 2024

amy-at-kickstarter Feb 12, 2024

scottkicks Feb 12, 2024

amy-at-kickstarter Feb 12, 2024

amy-at-kickstarter Feb 12, 2024

amy-at-kickstarter Feb 12, 2024

scottkicks Feb 12, 2024

amy-at-kickstarter Feb 12, 2024 •

edited

scottkicks Feb 13, 2024

scottkicks Feb 12, 2024

amy-at-kickstarter Feb 12, 2024

scottkicks Feb 12, 2024

scottkicks Feb 12, 2024

amy-at-kickstarter Feb 12, 2024

amy-at-kickstarter Feb 12, 2024

scottkicks left a comment

	let title = $0.1
	PaginationExampleProjectCell(title: $0.1)
	PaginationExampleProjectCell(title: $0.1)

		let onRefresh: @Sendable() -> Void
		let onDidShowProgressView: @Sendable() -> Void


		You can observe the results of `values`, `isLoading`, `error` and `state` to access the loaded data.

		*/

MBL-1016: Create Paginator for pagination in SwiftUI/Combine #1939

MBL-1016: Create Paginator for pagination in SwiftUI/Combine #1939

Conversation

amy-at-kickstarter commented Feb 8, 2024 • edited

📲 What

🤔 Why

🛠 How

👀 See

Choose a reason for hiding this comment

amy-at-kickstarter Feb 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nativeksr commented Feb 8, 2024

scottkicks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-at-kickstarter Feb 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottkicks left a comment

Choose a reason for hiding this comment

amy-at-kickstarter commented Feb 8, 2024 •

edited

amy-at-kickstarter Feb 8, 2024 •

edited

amy-at-kickstarter Feb 12, 2024 •

edited