Consider adding isNetworkingError error type

[sakuraehikaru]

In one of our projects, we found ourselves often needing to identify networking errors, and so we've extended the error as such:

var isNetworkingError: Bool {
    switch errorCode {
    // NSURL Error Codes
    case NSURLErrorCancelled, NSURLErrorTimedOut, NSURLErrorCannotFindHost, NSURLErrorCannotConnectToHost, NSURLErrorNetworkConnectionLost, NSURLErrorNotConnectedToInternet, NSURLErrorSecureConnectionFailed:
        return true
    // POSIX Error Codes
    case Int(POSIXErrorCode.ECONNABORTED.rawValue), Int(POSIXErrorCode.ENETUNREACH.rawValue), Int(POSIXErrorCode.ECONNRESET.rawValue):
        return true
    default:
        return false
    }
}

This turned out to be quite useful, and maybe could be baked-in.

[sakuraehikaru]

Right now I think these system-level NSURL errors are captured and encapsulated in case requestFailed(Error), so in other words my suggestion is to add one or more specific error cases rather than lump them all together.

[nbrooke]

one or more specific error cases

I'd definitely vote for "more". In particular, cancelled problaby shouldn't be lumped in with other network errors (because it's almost always app requested in some way, often you don't even want to treat it as an error but as a different kind of intended completion). Not connected to the internet is another one that we might want separate, since it often gets some custom handling (maybe a custom error message, rather than a more generic one, since it's almost always up to the user to fix that, or maybe it's ignored as an error response because we know that reachability checking is gonna be warning on that).

[brendanlensink]

@JeremyChiang do you have a sense of how exactly we'd go about implementing this?

What error classifications would we use? Just isNetworkingError: Bool? Or do you think there might be some value in being able to specify a set of different kinds of errors, like Nigel mentioned?

[sakuraehikaru]

@brendanlensink @nbrooke What do you think about adding 3 new (and perhaps optional) completion blocks (1 for cancelled, 1 for not connected to internet, and 1 for connectivity issues) like Nigel has mentioned?

3 new completion blocks may seem a bit aggressive and weird at first, but they would provide the following benefits:

• reduce error parsing code
• make error handling code more readable , specific, and easier to understand
• reduce some callbacks altogether via passing and sharing of blocks

  netable.request(request, cancelled: showCancelledError, noInternet: goBackToLogin) { result in
  switch result {
      case .success(let response):
          completion(.success(message: response.message))
      case .failure(let error):
          // New architecutre
          // just need to handle "regular", mostly http errors, since other handling are passed as arguments
          completion(.failure(error: error))
          // Current architecture, option A
          // lots of error parsing and branching code
          if error == cancelled {
              showCancelledError
          } else if error == noInternet {
              goBackToLogin
          }
          // Current architecture, option B
          // not as clear, and these helper functions tend to bloat and destabilize over time
          self.handle(error)
  }
  }

  As for the optionality of these new blocks, I am undecided. There are pros and cons in either direction.

If not-optional:

• forces us to implement comprehensive error handling

If optional:

• easier for requests that don't need complete error handling
• allows us to decide when to opt-in to certain error handling

[brendanlensink]

I'm pretty strongly opposed to adding more completion blocks to request unless there's a good reason.

I had assumed this was more like adding some sort of classification to NetableErrors that allows users to filter out different classes of errors in their local/global error handlers.

Something like:

extension NetableError {
    var errorClassification {
        switch self {
            case a, b, c, ... : return .redFlag
            case l, m, n, ... : return .orangeFlag

        }

    }

}

This would let you do things like add a global error handler that only listens for red flag errors and reports them to remote logs, or something similar.

[nbrooke]

I don't love the idea of multiple callbacks either. The missing internet connection one is MAYBE approaching the level of it being worth that much ceremony (because that's a very common error that we don't always handle consistently and that almost all network calls probably should have somewhat special handling for), but for something like cancellation, the vast majority of requests wouldn't need to do anything specific with it, having a specific call back in every request call would be huge overkill for that. And if we come up with other categorization the callback environment gets way more complicated quickly.

Multiple callbacks also open the can of worms of how to handle overlap between the callbacks. If it has a network error does it call BOTH the network error callback and the regular callback. If not, then cleanup code may need to be duplicated (as discussed in the context of splitting success and failure callbacks here: https://github.com/steamclock/netable/issues/79#issuecomment-904237568). If it does, then you can end up with the error being handled in BOTH places unless you specifically exclude it, i.e. the above code would need to look something more like:

case .failure(let error):
    if !error.isCancelled && !error.isNetworkError { // other callbacks are handling these    
            self.handle(error)
    }
}

which is getting a lot less compelling in its simplicity.

Multiple callbacks also wouldn't be compatible with future forms of the request functions using Combine publishers or async. In both those cases the library / language limits you to only dealing with two result values (a value and an error)

Also note that the global error handling (https://github.com/steamclock/netable/issues/79) should make some of the cases where you would use shared callbacks a lot easier.

I do think properties indicating classes of common errors is the way to go here.

[sakuraehikaru]

Thanks @brendanlensink and @nbrooke. Okay, let's go with the classification route. Who is going to implement this, and do we need any further discussion to determine the classification and its details?

[brendanlensink]

If you'd like to, I think it would make more sense to have someone go off and take a first pass at implementing this, then we can discuss any changes on that PR rather than discussing it first.