Investigate ways to further reduce the need for custom request wrapper functions

[nbrooke]

One of the design goals for Netable was to reduce the need for "wide" enums or function interfaces to specify sets of operations, stuff like:

enum Endpoint {
  case getFoo
  case setFoo(Foo)
  case getBar 
  case setBar(Bar)
  //... dozens of other endpoints
}

class Api {
  func getFoo(completion: (Foo?) -> Void) {
     request(Endpoint.getFoo) {
        //...
     }
  }

  func setFoo(_ foo: Foo, completion: (Bool) -> Void) {
     //...
  }

  // ... dozens of other functions 
}

Netable has been quite successful at reducing uses of the first, but much less successful at reducing uses of the second. Adding the finalization logic removed one important source of boilerplate in those sorts of API wrappers, but there's still enough stuff that you want to be done in a common way per request but that the request interface doesn't capture, that there is often need for those sort of wrappers. Some common ones we see a lot:

• We want some common global or semi-global data to be set on certain types of requests. "Every time we read the user put that in LoginManager.currentUser".
• We want local common error handling of some type. "404 errors on this request get transformed into another error type because that has some special meaning here"
• We want some sort of global common error handling. "All of these apis should generate a warning error toast when network errors happen on them".

Assuming we still think that boilerplate is worth reducing further (which is a question we should ask first, rather than just charging ahead with this, in particular this question does interact with some of our thinking on how repositories and services should be structured in the overall architecture, which has evolved a fair bit since Netable started), we should do a bit more systematic of an audit of what the full set of things that still require that kind of point of functionality that can't fit into the request objects, and look into ways to specify those bits of behaviour as part of the request.

[nbrooke]

specify those bits of behaviour as part of the request

One follow up though, I say the above, but in fact some of the functionality might be better NOT as part of the request. Global error handling, in particular, might make more sense as some callback (or Combine publisher, or whatever) on the Netable instance rather than as part of a request.

[brendanlensink]

It's been my experience with past projects, that we usually run into a situation where we've got a giant Manager or Model class full of thin wrappers on netable.request due to one of a couple situations:

1. There's a bunch of ViewModel or Model classes that all need to use the same/similar requests, so they can't directly call netable.request without creating a lot of redundant code.

2. We need to do some kind of complex post-processing with the request result, or failure, that doesn't really fit into the ViewModel without becoming a pyramid of doom really quickly.

Additionally, I think we've got two other issues in the pipeline that Nigel's mentioned that could influence this pretty heavily:

1. Supporting global error handling
2. Changing our philosophy at Steamclock to more heavily use Repositories to group blocks of code that interact with similar data

Finally, I think that there's a chance that while we'd like to avoid these custom request wrapper functions, doing so at the expense of larger blocks of code in ViewModels/other places is probably a net loss for a codebase.

With all that being said, here's what I think makes sense as our next course of action:

• Encourage users to group similar requests into Repositories, with separate Netable instances where appropriate. Note that this will likely result in repositories being a big collection of fetch/publishers for each piece of data, which isn't ideal and sort of the problem we originally set out to fix, but I think the lesser of two evils :/
• Change onCompletion to onSuccess() and onFailure?() with onFailure falling back to a netable-level default error handling (covered in #79)

[nbrooke]

To recap discussion from a sync meeting on this, general consensus seem to be that our internal architecture plans to move more in the direction of Repository objects for managing data somewhat obviates the need for any changes here. Repositories would wrap a lot of the complexity of this internally, but would also be less boilerplate-y, since they are going to be doing more internal processing, caching and so on, but repositories could problaby use Netable directly rather than needing to wrap it behind another abstraction. So the idea here is to sit on this for a while and see how that works in practice, and then revisit in a while once we've done more with that architectural style.

Looking back over previous discussion, I did come up with one more concrete feature that could add that could help with this a little more, and is also maybe a useful thing to support on it's own. Going to file that separately.

[nbrooke]

Filed as #92

[amyoulton]

Hey all!

I've been assigned this ticket and there been a lot of conversation, and it seems the last bit of convo was "we'll think about this later, with the except of ticket #92", which I have done to some extent and have asked for some mentorship on.

To me having just come into this, it seems a lot of what this is discussing now exists (with the exception of #92). So my question is what is the scope of this ticket and what is it you're wanting me to tackle here?

[nbrooke]

Yeah, I don't think there are any other concrete tasks here until we've had a little more discussion on it.

Looking at some recent examples of usage, it seems like this claim of mine at least: "but repositories could probably use Netable directly rather than needing to wrap it behind another abstraction", has not proven true in practice, that a lot of repositories in our current apps do generally have an associated service that seems to consist of exactly the sort of extremely thin wrapper around Netable that we say we are trying to avoid. It looks like the main reason for that now is maybe testability (at least in the most modern of the projects using the architecture). Like the service has an interface and a concrete implementation that is near trivial, but we need the abstraction to be able to replace the trivial implementation against Netable with a less trivial, I guess the open questions now are:

• Is testability the main reason we sill need to create thin wrappers?
• If so, is there some alternate approach to testabilty we can take (maybe the ability to mock requests at the Netable level in some way?) that would let us dispense with the current "null" services?

[brendanlensink]

Is testability the main reason we sill need to create thin wrappers?

IMO yes, the biggest responsibility for repos right now in my mind (other than passing data between VMs) is negotiating caching and handling the logic for dealing with cached/fetched data.

I think if we were able to come up with some alternate way to ensure that we're able to test the caching logic and fetching from Netable separately, we might be able to remove or significantly reduce the size of the service layer in our apps?

[amyoulton]

It's harder for me to comment on this because my own experiences with implementing Netable is so limited. That being said:

I would like to clarify what is meant by "thin wrappers". Looking into the term, the consensus on the term "wrapper" can slightly vary but essentially boils down to something like a class that wraps the functionality of another class. In the case we're talking about, I believe you're referring to the service layer wrapping the netable functions, which are then used by the repositories.

Is my understanding there correct? Is there an area I can look into more to get a deeper understanding of these terms, and can you clarify what "thin" means?

Also, what should our next steps be with this ticket. Is there anything I am able to do to move things forward?

[nbrooke]

Yeah, that's a correct understanding of what we mean by that. In particular "thin" or "boilerplate" wrappers are ones where the fact that it is wrapped tends to not provide a lot of value because the wrapping is trivial, something like (A slightly modified example from a client project you may be familiar with 😄 ):

protocol UserServiceProtocol {
    func getCurrentUser(id: String) async throws -> User
    func getPaymentHistory(id: String) async throws -> [PaymentItem]
    func getUserMessagingPreferences(id: String) async throws -> UserMessagingPreferences
    //...
}

class UserService: UserServiceProtocol {
    func getCurrentUser(id: String) async throws -> User {
        try await request(CurrentUserQuery(id: id))
    }

    func getPaymentHistory(id: String) async throws -> [PaymentItem] {
        try await request(PaymentHistoryQuery(id: id))
    }

    func getUserMessagingPreferences(id: String) async throws -> UserMessagingPreferences {
        try await request(MessagingPreferencesQuery(id: id))
    }

    //...
}

Doing this is necessary to make the network request mockable with the current architecture, but this does mean that every time you add a request, you need to touch three places (these two types and the request type itself), two of which are trivial, to add any new request. It would be better if we didn't have to add those and could just dispense with these wrappers and use the requests directly where the wrappers are being called now, but that requires some solution to how to mock network requests without using the protocol to swap in a new implementation of the wrapper.

[amyoulton]

I think this ticket needs a little more thought before any tasks can be done on it as to how to solve the test issue.

I don't have a great idea with how to proceed on this ticket, so am going to unassign myself from actively working on this right now, if that makes sense to you @brendanlensink.