[#324] Use Moya as the main Network Layer #394
Conversation
Shayokh144
requested review from
suho,
blyscuit,
edgarss,
markgravity,
minhnimble,
nmint8m,
phongvhd93,
vnntsu,
ducbm051291,
Thieurom and
nkhanh44
as code owners
| } | ||
| .asSingle() | ||
| provider.rx.request(configuration) | ||
| .filterSuccessfulStatusAndRedirectCodes() |
There was a problem hiding this comment.
Correct me if I'm wrong. I don't think that we need to filter the status code here. If there's a server error with code 5xx, will this request have no response?
filterSuccessfulStatusAndRedirectCodes() filters status codes that are in the 200-300 range.
| var url: URLConvertible { get } | ||
|
|
||
| var parameters: Parameters? { get } | ||
| /// Add cases for each endpoint |
There was a problem hiding this comment.
No need for documentation comments, it's ok to see on Moya's repository.
|
|
||
| var headers: HTTPHeaders? { nil } | ||
| /// Return stub/mock data for use in testing. Default is `Data()` | ||
| var sampleData: Data { Data() } |
There was a problem hiding this comment.
No need to include optional vars.
|
|
||
| var interceptor: RequestInterceptor? { nil } | ||
| /// Return the type of validation to perform on the request. Default is `.none`. | ||
| var validationType: ValidationType { .successCodes } |
There was a problem hiding this comment.
This has a default value, user of the repo can modify this by themself.
|
|
||
| var headers: HTTPHeaders? { get } | ||
| /// Return base URL for a target |
There was a problem hiding this comment.
No need for documentation comments, it's ok to see on Moya's repository. And for other comments here.
suho
changed the title
|
|
||
| protocol RequestConfiguration { | ||
| enum RequestConfiguration { |
There was a problem hiding this comment.
RequestConfiguration was a protocol to define a blueprint for every request. It couldn't be an enum or struct like this since we will have a specific config for each group of APIs based on the endpoint, e.g user -> UserRequestConfiguration.
It's also great if we keep RequestConfiguration as its responsibility with Alamofire, which means it's like a wrapper of Moya. Let's say we will not use Moya and change to another one in the future, so we don't need to update every "RequestConfiguration" to conform to the new lib. Conversely, we're going to import Moya to every request config to be able conform TargetType (and yeah, now it should be named ABCTarget).
There was a problem hiding this comment.
@vnntsu I am a bit confused about this.. Are you suggesting to keep the RequestConfiguration protocol unchanged?
There was a problem hiding this comment.
@vnntsu We're using Moya, so I think using Enum will be fine. In case we have different groups of APIs, we can create another RequestConfiguration like UserRequestConfiguration, as long as we implement the protocol TargetType
There was a problem hiding this comment.
@Shayokh144 I suggest keeping RequestConfiguration protocol as a wrapper of TargetType; otherwise, we remove it.
The enum RequestConfiguration is redundancy. We will have specific request configs such as UserRequestConfiguration or OtherRequestConfiguration, etc.
There was a problem hiding this comment.
Before Moya, our RequestConfiguration is TargetType, and it's a wrapper for Alamofire. We are moving to Moya, so I prefer to remove RequestConfiguration.
@vnntsu did raise a good point about changing to another networking library in the feature. We might need to update every RequestConfiguration to conform to the new library. In this case, I prefer to have a typealias:
typealias TargetType = Moya.TargetTypeLet's say if we decide to remove Moya and go back to Alamofire, then we just need to implement the protocol TargetType (with baseURL, method...) in the network layer.
P.s: The reason I suggest typealias TargetType = Moya.TargetType instead of typealias RequestConfiguration = Moya.TargetType because it's more clear, using RequestConfiguration might confuse developers when we are using Moya.
There was a problem hiding this comment.
@suho typealias added and RequestConfiguration is deleted
{PROJECT_NAME}/Sources/Data/NetworkAPI/Core/NetworkAPIProtocol.swift
Outdated
Show resolved
Hide resolved
Shayokh144
force-pushed
the
chore/324-use-moya-as-network-layer
branch
from
4fc943c to
b66fafb
Compare
Shayokh144
force-pushed
the
chore/324-use-moya-as-network-layer
branch
from
b66fafb to
d37a1e0
Compare
|
@blyscuit @ducbm051291 I have updated the PR, please take a look |
{PROJECT_NAME}/Sources/Data/NetworkAPI/Core/NetworkAPIProtocol.swift
Outdated
Show resolved
Hide resolved
| import RxSwift | ||
|
|
||
| final class NetworkAPI: NetworkAPIProtocol { | ||
|
|
||
| private let decoder: JSONDecoder | ||
| private let provider: MoyaProvider<MultiTarget> |
There was a problem hiding this comment.
@Shayokh144 Regarding this discussion, it's better to remove NetworkAPI and NetworkAPIProtocol in this PR
|
@Shayokh144 we will have this initiative Replace-Network-Layer-with-Moya to work on before making the change to our ios templates. So I'll close this PR. |
#324
What happened
Replace
AlamofirewithMoyaas main network layer of the template.Insight
Alamofirefrom PodMoyain PodMoyaRequestConfigurationas it is not needed, developers can create their own request configurationenumextending Moya'sTargetTypeProof Of Work
Created a demo app from this branch, request-response working fine
Create different configurations independently
Sample configuration 1:
Sample configuration 2:
Call using any configuration with any decodable type
Write test with stubbed provider