RFC - Remove asURL() method and AppEnvironment base URL access

[mokagio]

@kean @crazytonyli I need to move the convenience init for WordPressOrgRestApi init?(blog:) because it's implemented in Swift but used in Objective-C...

I have two suggestions:

• Remove the asURL() usages that come from Alamofire
• Bypass AppEnvironment to read the WordPress.com base URL

I open this PR as an RFC so it'll make more sense in code.

Keen to hear what you think..

---

This is part of #24165

[dangermattic]

	1 Message
📖	This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

[mokagio]

asURL() comes from Alamofire

extension String: URLConvertible {
    /// Returns a `URL` if `self` can be used to initialize a `URL` instance, otherwise throws.
    ///
    /// - Returns: The `URL` initialized with `self`.
    /// - Throws:  An `AFError.invalidURL` instance.
    public func asURL() throws -> URL {
        guard let url = URL(string: self) else { throw AFError.invalidURL(url: self) }

        return url
    }
}

I makes sense as a util, but not if we need to carry Alamofire with us for it.

I considered reimplementing it is as something like:

    func asURL() throws -> URL {
        guard let url = URL(string: self) else { throw ToURLConversionError(string: self) }

        return url
    }

But then I realized that URL(string:) returns a URL for all string inputs I threw at it, see the test below in the diff for a little proof of concept.

Realistically, it's unlikely we'll have malformed URLs going through that code. But even if we did, URL(string:)'s behavior is such that it would not reject them, so there's little point in wrapping it in a throwing function...

My suggestion therefore is to get rid of these calls. At least in the code we want to move away from where Alamofire is already available.

[kean]

My suggestion therefore is to get rid of these calls. At least in the code we want to move away from where Alamofire is already available.

Absolutely!

Btw, I recently reworked the remaining code that was using AlamofireImage and ultimately removed it. It doesn't seem like there is much that's stopping us now from removing Alamofire as well.

[mokagio]

Before, we were using AppEnvironment.current.wordPressComApiBase but that's not available outside of the app targets.

Looking at the implementation, it looks like the only value of reading from there is because it can be changed at runtime somehow—but it would have to change before it's read in this init. Correct me if I'm wrong.

WordPress-iOS/WordPress/Classes/Utility/Environment/AppEnvironment.swift

Lines 45 to 52 in 9462b91

	extension AppEnvironment {
	/// Creates a new Environment, changing just a subset of the current global dependencies.
	///
	@discardableResult
	static func replaceEnvironment(
	appRatingUtility: AppRatingUtilityType = AppEnvironment.current.appRatingUtility,
	contextManager: CoreDataStack = AppEnvironment.current.contextManager,
	wordPressComApiBase: URL = AppEnvironment.current.wordPressComApiBase) -> AppEnvironment {

With that in mind and with the intention of moving this to WordPressData, then I think it makes sense to use the value it's initialized with:

WordPress-iOS/WordPress/Classes/Utility/Environment/AppEnvironment.swift

Line 37 in 9462b91

wordPressComApiBase: URL = WordPressComRestApi.apiBaseURL) {

[kean]

Makes total sense. AppEnvironment is largely unused (for now). It also appears to be a container for dependency injection and not a configuration manager, so wordPressComApiBase doesn't belong there.

[mokagio]

Thanks for the input @kean! I'll execute on this today then.