Moya: Missing case in Task enum

[SeRG1k17]

In one of my projects, I needed to use the parameters in the query string, and also use the body. Due to the fact that the property path can not be represented as:

public var path: String {
        switch self {
case .myCase(let name): return "api/endpoint?username=\(name)"
}

Here's what my case should look like:

case requestJSONEncodable(Encodable, parameters: [String: Any], encoding: ParameterEncoding)

[sunshinejr]

I think that this proposal makes sense. We usually name the cases using composable keyword, so in this case, it could be something similar to requestCompositeJSONEncodable(encodable: Encodable, urlParameters: [String: Any]). The implementation could also be similar to requestCompositeData.

I'd love to hear other @Moya/contributors what do they think about it :)

[SD10]

Ouch 😢 In my opinion, this is a drawback of using the current Task abstraction. It's a slippery slope to keep adding different cases. Nevertheless, I don't see a good alternative for the client to work-around this so I think this additional case is something we should add.

[ashfurrow]

Wait, I'm not quite clear on the problem. Both your request body and query string need the same parameters? Could the requestClosure be used for this? Or could the logic be abstracted into shared code?

[SD10]

I also want to make the argument that Moya should allow users to handle the mapping of an Endpoint to a URLRequest from within the Endpoint object itself

The mapping is currently defined as a public method:
https://github.com/Moya/Moya/blob/4555c7956e5acfb81bf759476c905f0866e5df4c/Sources/Moya/Endpoint.swift#L78

You can use the requestClosure of MoyaProvider to handle this mapping, however, this closure receives an Endpoint so the underlying TargetType has already been abstracted away. This makes it pretty difficult to provide any additional context that is available in the TargetType and not Endpoint.

You would need to:

1. subclass Endpoint to provide the additional context you want from TargetType
2. then provide your own endpointClosure to map to your new subclass
3. then provide your own requestClosure to handle mapping to Result<URLRequest, MoyaError>.

IMO the burden can be reduced by:

1. Changing Endpoint.urlRequest() from public to open
   
   
   
   • This eliminates the need for step 3 above.
   
   
2. Adding a urlRequestClosure argument to the Endpoint initializer.
   
   
   
   • This allows the user to control mapping to a URLRequest directly inside of the endpointClosure (where they still have access to the underlying TargetType). No need for a requestResult closure and no need to subclass Endpoint.
   
   

Sorry to drop this bomb 😢

[ashfurrow]

@SD10 There is no need to apologize! We can work through these things together as they come up – and things like this _do_ come up, for every project.

[SeRG1k17]

@SD10, @ashfurrow @sunshinejr Off-topic. I found another improvement. For example: we need a different timout for each request. Sure we need our manager. However, a typical timer change is described in #743. And in the last comment the author put forward a hypothesis. On adding one more case to the enumeration.

I believe we should reconsider some of the typical concepts that most developers face. After all, using closures is not always elegant.

[SeRG1k17]

@ashfurrow @SD10 @sunshinejr I need case for Encodable body and parameters as queryString.
Because "api/endpoint?username=(name)" in path property not working due to encoding. And even if it worked, the solution is very bad, i think.

    case requestJSONEncodable(Encodable)
    case requestParameters(parameters: [String: Any], encoding: ParameterEncoding)
    case requestCompositeData(bodyData: Data, urlParameters: [String: Any])
    case requestCompositeParameters(bodyParameters: [String: Any], bodyEncoding: ParameterEncoding, urlParameters: [String: Any])

None of them allow us to use Encodable + [String: Any] as url parameters. Sure i can use:

    public var task: Task {
        switch self {
        case let .setGlucoseThresholds(_, glucoseThresholds):
            let encoded = try! JSONEncoder().encode(glucoseThresholds)
            return .requestCompositeData(bodyData: encoded, urlParameters: parameters)
        }
    }

But I do not think this is a good decision. Because, this is not the place where we need to encode Encodable to data.

Update my proposal case:

case requestJSONEncodable(Encodable, urlParameters: [String: Any])

[SD10]

Hey @SeRG1k17,

I think it's fantastic that you have so many ideas to improve Moya 💡. If you could try and keep issues on-topic in the future that would be much appreciated. Keeping things on topic helps users easily find the information they're looking for. So if you have any ideas for improvements, please don't hesitate to open a separate issue 😊

About your suggestion, unfortunately, I'm against expanding the Task enum to support this case because I feel like providing the same data in both the request body and the query parameters is an uncommon use case.

I've thought of your suggestion to consolidate the cases:

-  requestJSONEncodable(Encodable)
+ requestJSONEncodable(Encodable, urlParameters: [String: Any])

However, I think having the extra associated value would annoy the majority of users who don't need it.

My suggestion is to convert your Encodable model to Data directly before assigning it to the .requestCompositeData(bodyData:urlParameters), as you demonstrated above.

cc @sunshinejr Thoughts?

[SeRG1k17]

@SD10 I want to not replace, but add a case. An example made me do a forced unwrapping data of meaning. Moreover, it is not possible to handler an error here.

[sunshinejr]

So did we come back to the solution I gave at the beginning? 😄 I also do agree that we shouldn't really replace the case with additional parameter 👍

[SeRG1k17]

@sunshinejr See above, friend 😃

[sunshinejr]

@SeRG1k17 Yeah, yeah, I just said that we all agree that replacing the current task is a no-no, and asked if you guys like the solution I gave at the beginning 😄

[SeRG1k17]

@sunshinejr Oh sorry. I did not notice. Yes, in fact, this is the same thing that you wrote above.

[SD10]

@SeRG1k17 I'm sympathetic to not being able to handle the encoding error. That is in fact, a real problem 🤔

Can you add the parameters in the baseURL directly by making it dynamic and then ignore the path component for this case?

I just think that if we're going to add another case for this, we need to decide where we either draw the line or need to rethink things 😭

[SeRG1k17]

@SD10 I can do that. But this seems like a "crutch".

    public var baseURL: URL {

        var url = "https://example.com"

        switch self {
        case .custom(let value): url += "?value=value"
        default: break
        }
        return URL(string: url)!
    }

[ashfurrow]

Could this be something that Parameter Encoding solves? We have docs here: https://github.com/Moya/Moya/blob/master/docs/Examples/ParameterEncoding.md Basically you can provide a custom bit of code that translates the parameters of the endpoint into the request. Again, a bit of a clutch, but as pointed out this is an uncommon scenario.

Moya should make common things easy and uncommon things possible, and a "clutch" in this case might be worth it for the sake of a smaller API surface area.

[pedrovereza]

On a side note, can we have this in the Vision document?

Moya should make common things easy and uncommon things possible

[ashfurrow]

Good idea: https://github.com/Moya/Moya/pull/1659

[stale[bot]]

This issue has been marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale[bot]]

This issue has been auto-closed because there hasn't been any activity for at least 21 days. However, we really appreciate your contribution, so thank you for that! 🙏 Also, feel free to open a new issue if you still experience this problem 👍.

[SD10]

This is a really long issue but for the sake of record keeping I want to make a few notes:

• You _cannot_ do this through the requestClosure because the TargetType has been abstracted
• You _can_ do this through the use of a PluginType because you still have access to TargetType

@SeRG1k17 Addresses the issue of wanting to use Encodable to encode parameters and there is currently not a good way to handle such an error.

However, if we made the methods of the PluginType protocol throwing, then we could bubble up this error to the user when applying plugins.