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.

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

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

Closed
wants to merge 4 commits into from

Conversation

Shayokh144
Copy link
Contributor

@Shayokh144 Shayokh144 commented Nov 28, 2022

#324

What happened

Replace Alamofire with Moya as main network layer of the template.

Insight

  • Remove Alamofire from Pod
  • Add Moya in Pod
  • Update networking code to use Moya
  • Add comments for better readability
  • Test this branch by creating a separate app from it
  • Remove common RequestConfiguration as it is not needed, developers can create their own request configuration enum extending Moya's TargetType

Proof Of Work

  • Created a demo app from this branch, request-response working fine

  • Create different configurations independently

  • Sample configuration 1:
    Screenshot 2022-12-01 at 5 40 35 PM

  • Sample configuration 2:
    Screenshot 2022-12-01 at 5 39 23 PM

  • Call using any configuration with any decodable type
    Screenshot 2022-12-01 at 5 45 46 PM

  • Write test with stubbed provider

Screenshot 2022-12-02 at 4 21 01 PM

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

Copy link
Collaborator

@blyscuit blyscuit left a comment

Choose a reason for hiding this comment

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

Please check David's comment as well.

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


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.

@suho suho changed the title [#324] [RFC] Use Moya as the main Network Layer [#324] Use Moya as the main Network Layer Nov 29, 2022

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

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

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

@vnntsu vnntsu modified the milestones: 3.12.0, 3.13.0 Nov 30, 2022
@Shayokh144 Shayokh144 force-pushed the chore/324-use-moya-as-network-layer branch from 4fc943c to b66fafb Compare December 1, 2022 10:31
@Shayokh144 Shayokh144 force-pushed the chore/324-use-moya-as-network-layer branch from b66fafb to d37a1e0 Compare December 1, 2022 10:47
@Shayokh144
Copy link
Contributor Author

@blyscuit @ducbm051291 I have updated the PR, please take a look

import RxSwift

final class NetworkAPI: NetworkAPIProtocol {

private let decoder: JSONDecoder
private let provider: MoyaProvider<MultiTarget>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please work with @suho to clarify this comment

Copy link
Member

Choose a reason for hiding this comment

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

@Shayokh144 Regarding this discussion, it's better to remove NetworkAPI and NetworkAPIProtocol in this PR

Copy link
Collaborator

@blyscuit blyscuit left a comment

Choose a reason for hiding this comment

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

🚀

@vnntsu
Copy link
Contributor

vnntsu commented Dec 20, 2022

@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.
cc: @nimblehq/ios-chapter

@vnntsu vnntsu closed this Dec 20, 2022
@suho suho deleted the chore/324-use-moya-as-network-layer branch October 18, 2023 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Moya as the main Network Layer
5 participants