Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[#324] Use Moya as the main Network Layer #394

Closed
wants to merge 4 commits into from
Closed

[#324] Use Moya as the main Network Layer #394

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
653aa94
[324] [Chore] Use Moya as network layer
Shayokh144 Nov 28, 2022
7cfad00
[324] [Chore] Remove comments and default items
Shayokh144 Nov 29, 2022
d37a1e0
[324] [Chore] Remove RequestConfiguration and use Maya MultiTarget
Shayokh144 Dec 1, 2022
f7d5b3c
[324] [Chore] Add decoder and typealias
Shayokh144 Dec 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Podfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ target '{PROJECT_NAME}' do
pod 'SnapKit'

# Rx
pod 'RxAlamofire'
pod 'Moya/RxSwift'
pod 'RxCocoa'
pod 'RxDataSources'
pod 'RxSwift'
Expand Down
34 changes: 6 additions & 28 deletions {PROJECT_NAME}/Sources/Data/NetworkAPI/Core/NetworkAPIProtocol.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@
// NetworkAPIProtocol.swift
//

import Alamofire
import RxAlamofire
import Moya
import RxSwift

protocol NetworkAPIProtocol {
Expand All @@ -12,33 +11,12 @@ protocol NetworkAPIProtocol {
}

extension NetworkAPIProtocol {

func request<T: Decodable>(
session: Session,
configuration: RequestConfiguration,
decoder: JSONDecoder
provider: MoyaProvider<RequestConfiguration>,
suho marked this conversation as resolved.
Show resolved Hide resolved
configuration: RequestConfiguration
) -> Single<T> {
return session.rx.request(
configuration.method,
configuration.url,
parameters: configuration.parameters,
encoding: configuration.encoding,
headers: configuration.headers,
interceptor: configuration.interceptor
)
.responseData()
.flatMap { _, data -> Observable<T> in
Observable.create { observer in
do {
let decodable = try decoder.decode(T.self, from: data)
observer.on(.next(decodable))
} catch {
observer.on(.error(error))
}
observer.on(.completed)
return Disposables.create()
}
}
.asSingle()
provider.rx.request(configuration)
.filterSuccessfulStatusAndRedirectCodes()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.map(T.self)
blyscuit marked this conversation as resolved.
Show resolved Hide resolved
}
}
42 changes: 19 additions & 23 deletions {PROJECT_NAME}/Sources/Data/NetworkAPI/Core/RequestConfiguration.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,38 +2,34 @@
// RequestConfiguration.swift
//

import Alamofire
import Foundation
import Moya

protocol RequestConfiguration {
enum RequestConfiguration {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@Shayokh144 Shayokh144 Nov 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vnntsu I am a bit confused about this.. Are you suggesting to keep the RequestConfiguration protocol unchanged?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@suho suho Nov 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.TargetType

Let'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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@suho typealias added and RequestConfiguration is deleted


var baseURL: String { get }

var endpoint: String { get }

var method: HTTPMethod { get }

var url: URLConvertible { get }

var parameters: Parameters? { get }
/// Add cases for each endpoint
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for documentation comments, it's ok to see on Moya's repository.

}

var encoding: ParameterEncoding { get }
extension RequestConfiguration: TargetType {

var headers: HTTPHeaders? { get }
/// Return base URL for a target
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for documentation comments, it's ok to see on Moya's repository. And for other comments here.

var baseURL: URL { URL(string: "https://base_url")! }

var interceptor: RequestInterceptor? { get }
}
/// Return endpoint path for each endpoint case
var path: String { "" }

extension RequestConfiguration {
/// Return HTTP method for each endpoint case
var method: Moya.Method { .get }

var url: URLConvertible {
let url = URL(string: baseURL)?.appendingPathComponent(endpoint)
return url?.absoluteString ?? "\(baseURL)\(endpoint)"
}
/// Build and Return HTTP task for each endpoint case
var task: Moya.Task { .requestPlain }

var parameters: Parameters? { nil }
/// Return the appropriate HTTP headers for every endpoint case
var headers: [String: String]? { ["Content-Type": "application/json"] }

var headers: HTTPHeaders? { nil }
/// Return stub/mock data for use in testing. Default is `Data()`
var sampleData: Data { Data() }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has a default value, user of the repo can modify this by themself.

}
14 changes: 5 additions & 9 deletions {PROJECT_NAME}/Sources/Data/NetworkAPI/NetworkAPI.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,19 @@
// NetworkAPI.swift
//

import Alamofire
import Foundation
import Moya
import RxSwift

final class NetworkAPI: NetworkAPIProtocol {

private let decoder: JSONDecoder
private let provider: MoyaProvider<RequestConfiguration>

init(decoder: JSONDecoder = JSONDecoder()) {
self.decoder = decoder
init(provider: MoyaProvider<RequestConfiguration> = MoyaProvider<RequestConfiguration>()) {
self.provider = provider
}

func performRequest<T: Decodable>(_ configuration: RequestConfiguration, for type: T.Type) -> Single<T> {
request(
session: Session(),
configuration: configuration,
decoder: decoder
)
request(provider: provider, configuration: configuration)
}
}