[ApolloPagination] Remove async requirement from AsyncGraphQLQueryPager initializers

[Iron-Ham]

This is a breaking change, and would require a 0.2.0 release.

In order to make instantiation of the AsyncGraphQLQueryPager easier, we can remove the async requirement of the initializer if we also ensure that accesses to cancellables becomes atomic. This pull request primarily focuses on the removal of asynchronous initializers and the introduction of Task in the AsyncGraphQLQueryPager class and its related test classes. The most significant changes include the removal of await from AsyncGraphQLQueryPager initializers, the introduction of Task for subscribing to pager results, and the use of @Atomic for thread-safe mutation of cancellables.

---

Why?

Honestly, it just feels better to instantiate it this way: it opens up the reality of instantiating the object in a SwiftUI view model, and then using it in an asynchronous context via .task modifiers in the view as the async entry-point. We could technically do that by declaring a new Task to instantiate the pager in, but then we are relegated to an optional-only pager.

The effective change to SwiftUI view models is something like this:

Before:

@Observable final class MyViewModel {
  var pager: AsyncGraphQLQueryPager<[MyCustomModel]>?

  init() {
    Task {
      pager = await AsyncGraphQLQueryPager(...)
    }
  }
}

After:

@Observable final class MyViewModel {
  var pager: AsyncGraphQLQueryPager<[MyCustomModel]>

  init() {
    pager = AsyncGraphQLQueryPager(...)
  }
}

---

I ran the AsyncGraphQLQueryPagerTests suite 100 times with the address sanitizer:
✅ Executed 700 tests, with 0 failures (0 unexpected) in 74.311 (74.597) seconds

100 times with the thread sanitizer:
✅ Executed 700 tests, with 0 failures (0 unexpected) in 79.664 (79.981) seconds

---

Changes to AsyncGraphQLQueryPager:

• apollo-ios-pagination/Sources/ApolloPagination/AsyncGraphQLQueryPager.swift: Removed async from the initialization of AsyncGraphQLQueryPager and its related methods. The cancellables property has been made atomic to ensure thread safety. Subscription to the pager's results has been modified to store the cancellable internally. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]

Changes to AsyncGraphQLQueryPagerTests:

• Tests/ApolloPaginationTests/AsyncGraphQLQueryPagerTests.swift: Adjusted the tests to reflect the removal of async from the initialization of AsyncGraphQLQueryPager. [1] [2] [3] [4] [5] [6] [7]

Changes to PagerCoordinator+Erase:

• Tests/ApolloPaginationTests/PagerCoordinator+Erase.swift: Adjusted the tests to reflect the removal of async from the initialization of AsyncGraphQLQueryPager. [1] [2] [3]

Changes to GraphQLQueryPager+Convenience:

• apollo-ios-pagination/Sources/ApolloPagination/GraphQLQueryPager+Convenience.swift: Adjusted the convenience methods to reflect the removal of async from the initialization of AsyncGraphQLQueryPager. [1] [2] [3] [4] [5] [6]

[netlify]

👷 Deploy request for eclectic-pie-88a2ba pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	9002b2a

[netlify]

👷 Deploy request for apollo-ios-docc pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	9002b2a

[AnthonyMDev]

I like the intent of this PR, and if it works correctly, then let's ship it. But I'm concerned about creating race conditions with the cancellables.

[AnthonyMDev]

Does this introduce a possible race condition where the cancellable set is cancelled between the time this one is created and the time when it is inserted into the set?

[Iron-Ham]

It seems unlikely, but I'm also not sure if it's a consequential effect? All told, I'm unconcerned about the init calls (since in order for that cancellable to have been cancelled, the class would have had to be deallocated).

The subscribe call is a little more risky.

This is valid:

let cancellables: Set<AnyCancellable> = []
let cancellable: AnyCancellable = ... 
cancellable.cancel()
cancellables.insert(cancellable)

I can't think of a valid test-case around hitting this edge case. Do you have an example of a specific concern to help zero in on what we should be double-checking?

[Iron-Ham]

On further examination – I think that the only thing this change really does is remove the guarantee that we're setup to publish values prior to a subscribe being called.

i.e., with code like this:

let pager = AsyncGraphQLQueryPager(...)
pager.subscribe { ... } 

There's a chance the act of a subscriber creating an AnyCancellable to be internally managed by the AsyncGraphQLQueryPager is executed prior to the init setting up to forward results from the AsyncGraphQLQueryPagerCoordinator. There's no negative impact to this.

Even if a networking operation somehow begins and completes execution prior to the init Task completing – its data will still be forwarded the moment that the Task does complete.

[Iron-Ham]

This may be a better option than what's in main currently – especially for edge cases in SwiftUI. I haven't reproduced this, but this is a bit of a thought exercise.

Imagine we have a simple ViewModel that instantiates an AsyncGraphQLQueryPager, and has functions for fetch, refetch and loadNext:

@Observable class ViewModel {
  let pager: AsyncGraphQLQueryPager<[String]>!
  var names: [String] = []
  init() {
    Task {
      pager = AsyncGraphQLQueryPager(...)
    }
  }

  func fetch() async {
    await pager.fetch()
  }
  func refetch() async {
    await pager.fetch()
  }
  func loadNext() async {
    try? await pager.loadNext()
  }
}

Note the instantiation – we do so in a Task within the init.

A common pattern would be to use a top-level task on the top-level view to perform the initial fetch:

struct SwiftUIView: View {
  @StateObject var viewModel = ViewModel()
  var body: some View {
    List($viewModel.names) { name in
      Text(name)
        .task {
          guard name == viewModel.names.last else { return }
          await viewModel.loadNext()
        }
    }
    .task {
      await viewModel.fetch()
    }
    .refreshable {
      await viewModel.refetch()
    }
  }
}

There's actually more concern in this existing code than I would have for the proposed code. Specifically: the task modifier on the top-level view can be treated as analogous to viewWillAppear. What happens if we call viewModel.fetch(), and the view model's init's Task hasn't completed yet? There would be no pager to fetch from.

The proposed code would move the initialization of a pager outside of a Task. That means that in a similar scenario to the one above, the pager would begin fetching – and then forward data when the pager's init Task completes.

[AnthonyMDev]

Same question as below

[AnthonyMDev]

Thanks for all the elaboration on this!