Convert `await -> Result<T, Error>` functions to `await throws -> T`

[mokagio]

I recently submitted a PR to address a warning. @jkmassel spotted the method I operated in has signature await -> Result<Void, Error>, at which he commented "the use of async -> Result is unfortunate here".

I think what Jeremy is hinting at is that, while evidently compiling correctly, async and Result are somewhat clashing from a functionality point of view.

Background on Result and asycn/await

Prior to async/await, it was useful to use Result in asynchronous method to have a single callback that safely encapsulated both success and failure outcomes:

func someAsyncComputation(_ onCompletion: Result<Int, Error>)

With async/await, we have an alternative pattern available

func someComputation() async throws -> Int

Of course, the option used in the codebase also compiles, but it feels a bit stuck in the middle.

func someComputation() async -> Result<Int, Error>

Here's the difference at call site:

// Result in closure
someAsyncComputation { result in 
  switch result {
  case .success(let value): ...
  case .failure(let error): ...
  }
}

// async throws
do {
  let value = try await someComputation()
  // ...
} catch {
  // ...
}

// async throws in a function that itself throws to bubble up the error
let value = try await someComputation

// async Result
let result = await someComputation()
switch result  {
  case .success(let value): ...
  case .failure(let error): ...
}

Personally, I am a big fan of Result and I love seeing it in use. But I think it works best when chaining functional programming style transformations in railway oriented programming:

result
  .map { ... }
  .flatMap { ... }
  .mapError { ... }
  // etc

Where to go from here

At the time of writing, there are 24 matches for async -> Result in the codebase.

I sampled them and I don't see us leveraging Result in a railway oriented programming way. Example see loadUsernameSuggestions(from:) defined as

woocommerce-ios/Networking/Networking/Remote/AccountRemote.swift

Line 16 in d0a66f4

func loadUsernameSuggestions(from text: String) async -> Result<[String], Error>

and used as

woocommerce-ios/Yosemite/Yosemite/Stores/AccountCreationStore.swift

Lines 80 to 85 in d0a66f4

	let usernameSuggestionsResult = await remote.loadUsernameSuggestions(from: base)
	guard case let .success(usernameSuggestions) = usernameSuggestionsResult,
	let username = usernameSuggestions.first else {
	return nil
	}
	return username

We could rewrite this to something like

return try? await remote.loadUsernameSuggestions(from: base)?.first

It's also possible we'll find some calls that ought to remain callback-based and could benefit with Result without async.

---

I'd love to hear your thoughts on this. Thanks 😄

[peril-woocommerce]

	Fails
🚫	Please add a feature label to this issue. e.g. 'feature: stats'

Generated by 🚫 dangerJS

[jkmassel]

I think what Jeremy is hinting at is that, while evidently compiling correctly, async and Result are somewhat clashing from a functionality point of view.

Ah yep, sorry 😅

return try? await remote.loadUsernameSuggestions(from: base)?.first

My only nit: can we avoid try?, instead passing the error to the caller to provide additional information?

[crazytonyli]

I'm leaning towards using throw here. I do like the error type being part of the function signature, but at the same time it feels more "nature" to me to write a throwing function.

[koke]

+1 for async throws. I wanted Result for a long time and was happy when it came to swift, but mostly because we had to rely so much on completion blocks and you couldn’t throw there. It has served its role well, and it will for a while until we replace all completion blocks with async code, but async throws feels like the right way of doing things now

[mokagio]

@jkmassel

My only nit: can we avoid try?, instead passing the error to the caller to provide additional information?

For sure! The original code swallowed the error and I kept the pattern in my example. But the systematic migration from async -> Result to async throws would be a great occasion to up our game in terms of error handling.

@crazytonyli

I do like the error type being part of the function signature

Yeah, me too. Even though, since Result started allowing using the generic Error, I've noticed less precision in the type of error used within it.

I found this discussion regarding "typed throws" but it looks stale 😞

[mokagio]

I updated the title from a question to a command, to reflect the decision that has been taken.