Moya: Misleading of defaultAlamofireManager in MoyaProvider+Defaults

[zhongwuzw]

In MoyaProvider+Defaults.swift, has a defaultAlamofireManager method to get the default Alamofire's Manager. Alamofire's Manager has a static instance of default. But in defaultAlamofireManager's implementation, it just use defaultHTTPHeaders from Alamofire, would create a new Manager instance every call.

     // Moya
    public final class func defaultAlamofireManager() -> Manager {
        let configuration = URLSessionConfiguration.default
        configuration.httpAdditionalHeaders = Manager.defaultHTTPHeaders

        let manager = Manager(configuration: configuration)
        manager.startRequestsImmediately = false
        return manager
    }

    // Alamofire
    open static let `default`: SessionManager = {
        let configuration = URLSessionConfiguration.default
        configuration.httpAdditionalHeaders = SessionManager.defaultHTTPHeaders

        return SessionManager(configuration: configuration)
    }()

[SD10]

I gave this some thought and I think it makes sense to change this to a static property. Do any other @Moya/contributors have some input on this subject? I only joined the project recently so I may be missing context.

[freak4pc]

I definitely think this should be static, but ...
We need to consider there might be some people who actually count on this side-effect by now (which is wrong, but is the current behavior). So if we make this change, it would be a breaking change for some people.

The worst part is some people won't necessarily notice it would break their codebase since its a silent fix.

Not sure how common this scenario is, but its definitely worth considering.

[AndrewSB]

@freak4pc brings up a great point. Those silent breakages are the most dangerous in my opinion. If we decide to make this change I recommend artificially making a breaking change in the source with a @deprecated message that shows up as a warning so people see that at upgrade time.

I’ve also asked a friend who’s a consumer of Moya, currently back on v8 for his opinion on this. I’ll post here when we’re done talking

[ashfurrow]

Hmm. Let's step back for a minute before discussing changing Moya's behaviour.

The reported issue is that the method name is misleading, so let's discuss that first.

The method is scoped to a file that begins with Moya+, so it's semantically scoped to the Moya project. Additionally, the rest of the file is all functions for defaults _of Moya_, so again it fits well. For historical context, the defaultAlamofireManager function itself predates Alamofire's concept of a "default" (previously called "shared") manager. We used a function here specifically because it _does_ create a new Alamofire manager instance with each invocation (if it were accessing a shared instance, I'd agree that using a static var would be better).

So we have a function with a slightly misleading name, but otherwise the function fits well within Swift idioms. I think we should consider two possibilities:

• Adding documentation explaining that this function doesn't return the default Alamofire manager instance.
• Renaming the function to avoid ambiguity with Alamofire's default manager.

I'm very concerned about code churn in stable projects like Moya; imo, any breaking change needs to be justified by tangible benefits to Moya users because so many developers depend on this library and I don't want them to feel frustrated. Which of the two options we pick (if either, or both) should depend on feedback from the community. @zhongwuzw and @SD10's friend's input would be valuable here.

[SD10]

@freak4pc Thanks for mentioning the repercussions for those relying on this behavior. I had similar concerns, less so because the method is marked public, but more so now that I realize someone could wrap this call in another function making modifications or modify the property of the provider directly. 👍

I want to say that this should be made a static var but after @ashfurrow's comments I'm thinking otherwise:

1. Does it really make sense to have a 1 to many relationship between Manager and Provider?
   I think the current 1:1 relationship may make more sense, to be honest.

2. Normally, Manager would be considered a candidate to be made static to eliminate the overhead of initialization. It seems like this is not a huge concern, as no Moya consumer has mentioned it before.

I mean, Ash has made a good case as to why this is a function and it is prefixed default. It really makes sense if you read between the lines. (It's not like it's named sharedManager 🤔)

I would opt for adding a comment unless we have overwhelming support to change this.

[freak4pc]

If this would be written from scratch today I would probably opt for a singleton style var, but since this is already a relatively mature codebase and the added value to this change would be minimal, I don’t think it’s necessary.

We could have a purely additive change where we add a ‘default’ computed static var, and whoever wants can use either, possibly.

[AndrewSB]

@ashfurrow your thoughts are always awesome. Thank you for taking the time to step in on this ❤️

My friend is more of a professional friend, he is almost definitely going to respond, but I'm not going to push him to respond right now due to the chance of him taking time off for the holidays. His opinion should be useful for more than just this issue. I was very curious when I heard they were on a 10+ month old release for an iOS app which is currently in active development

[ashfurrow]

Cool! Looking forward to hearing. Of course, everyone is welcome to chime in, too.

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

[AndrewSB]

I got a response back a while ago, forgot to forward it

We haven’t upgraded it yet mostly because of time constraints because we wanted to get our app out the door. We were in crunch mode. We’d like to update it soon-ish (within a month or so) if possible.

So it's because their team cares prioritizes their product working over keeping their dependencies upto date.

[SD10]

I'd agree to add a sharedAlamofireManager that is a static var on top of the existing defaultAlamofireManager() function.

The problem is this is such a trivial detail I think it would be really difficult to communicate to Moya users that they can use the sharedAlamofireManager through documentation.

And if we make the change to using sharedAlamofireManager by default, this is a silent change in behavior. I don't think that's good either.

[zhongwuzw]

I agree @SD10 's thoughts, we can add a sharedAlamofireManager var, and add some explain about 1 to 1 relation for the default currently, and add the usage of sharedAlamofireManager if people prefer just one shared manager.

[SD10]

The way I see it I think we can either add a static var sharedAlamofireManager and replace this as the default in MoyaProvider or we can just add this property as a purely additive change. Then document that this is available to use. However, it is likely that not many people would find it even with proper documentation. So I guess it comes down to do we want to make this purely additive or do we want to change default behavior?

_{Sent with GitHawk}

[sunshinejr]

If I could add my two cents here, both of these options doesn't feel very appealing to me. Having two managers, sharedAlamofireManager and defaultAlamofireManager, would be very confusing to me at first. And having in mind the fact that the only difference between them is that one is a stored and one is a computed property, is not a big enough factor to introduce this unnecessary complexity.

If someone wants this manager to be a stored one, the fix is fairly simple. We could document it more, but that's all I would do.

[SD10]

More context on this issue. Looks like this setup is due to this comment