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

Feature/swift result #2769

Closed
wants to merge 4 commits into from

Conversation

AtomicCat
Copy link
Contributor

For Review Only - See notes in Implementation Details

Issue Link 🔗

#2752

Goals ⚽

This PR removes the custom Result<Value> type and replaces it with a typealias to the new Swift 5 Result type: typealias AFResult<T> = Result<T, Error>.

The existing extensions on Result are also updated to work with AFResult

In addition, all internal references to Result have been replaced with references to AFResult

Implementation Details 🚧

The goal was to simply replace the existing Result type and retain the extended functionality as well; this would remove a now redundant type and keep the existing functionality that the code relied upon.

However in the process of implementing the changes I've come to realize this may not be the best course of action.

  • Ideally the extensions on AFResult would be namespaced behind an af sub-struct as outlined in [Alamofire 5]: An Alamofire Namespace #2750. However, I was unable to come up with any Swift syntax that actually compiled as expected due to the generic nature of Result. Perhaps someone else will have better luck than I.

  • In preserving the existing extensions which the Alamofire code depends on, there are impedance mismatches with the new Swift Result type. For example, both implementations have a flatMap and flatMapError functions, but the semantics and the signatures of the supplied closures differ. The AF implementations take a closure that yields the Success or Failure types and can throw, whereas the Swift variants yield a new Result and cannot throw. There is existing code that relies on the old behaviors, so simplifying to the Swift versions is potentially non-trivial.

  • A number of the existing Result extensions were explicit in the docstrings about returning an unmodified Result (e.g. return self), but it's not obvious why that was called out. I was unable to bridge the type mismatches with the new code and in many cases it now returns new AFResult instance with the expected value.

  • Given the lack of extension namespacing, it is unclear if publicly extending the Swift Result is a good idea. While, switching to an internal access level would allow the existing code to function as-is, it would break any client code relying on the old functionality.

Testing Details 🔍

The unit tests for AFResult pass, though I have a lot of random failures on other unrelated tests. The master branch also has a number of similar failures. The corporate network I'm on is pretty locked down, so that could be a factor.

@AtomicCat
Copy link
Contributor Author

AtomicCat commented Mar 27, 2019

I thought this would be easy (ha), but I think it's revealed that some architectural/philosophical decisions need to be made by people closer to the project than I am.

@jshier
Copy link
Contributor

jshier commented Mar 27, 2019

You'll need to merge master to fix the conflicts. (Fun!)

If we can't namespace the extensions I would just get rid of them and use our typealias to cover the API. There are few internal bits that will need to change but I'm not concerned about breaking Result usage. It's unfortunate but necessary. We could manually prefix the methods with af_ but I think I'd want to remove everything first and see what we want to keep.

@cnoon
Copy link
Member

cnoon commented Mar 27, 2019

I agree @jshier. I think we should remove public extensions on Result so that we only vend AFResult as a typealias with no additional functionality, and keep any internal extensions that are necessary. I'd also hope though we'd work to remove the dependence internally on old, extended APIs on AFResult and instead move over to the new APIs that we have available.

Copy link
Member

@cnoon cnoon left a comment

Choose a reason for hiding this comment

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

Really good overall. Certainly some things we'll probably need to work ourselves to get the extended APIs moved to internal or possibly eliminated. IMO we just need a rebase and then we can move it through. Then @jshier or I can work on getting things moved to be internal.

Source/AFResult.swift Show resolved Hide resolved
Source/AFResult.swift Show resolved Hide resolved
@cnoon cnoon requested a review from jshier March 27, 2019 20:44
@cnoon cnoon added this to the 5.0.0-beta.4 milestone Mar 27, 2019
@jshier
Copy link
Contributor

jshier commented Mar 27, 2019

Ultimately I think we want to remove all extensions except those we use internally, those too if we can replace them with functionality from Swift.Result. When I looked at this it was only the properties that didn't have an easy equivalent, so you can cut out everything except that which breaks the build and we can reexamine.

Add AFResult as a typealias to Result.
Convert Result extensions to AFResult type.
Update all code to use AFResult instead of Result.
… direction is to remove them.

Add @testable to some unit tests that depend on functional extensions until they can be removed.

import Foundation

public typealias AFResult<T> = Result<T, Error>
Copy link
Contributor

@ejensen ejensen Mar 28, 2019

Choose a reason for hiding this comment

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

Can the Failure case be changed to the more specific AFError type?

I believe Alamofire always vends AFErrors currently masked behind Error.
Which requires receivers to implicitly know that they should use the error.asAFError extension to get the detailed error type.

The switch to explicitly specifying the Failure case as AFError would eliminate the need for the extension on Error and the implicit knowledge of its existence

Copy link
Contributor

@jshier jshier Mar 28, 2019

Choose a reason for hiding this comment

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

Not really, no, as we don't convert underlying system errors to our error type in most situations. It would also require us to wrap all user errors as well, as we don't wrap things like serializer failures in our own error type either. And even if we did wrap everything in AFError, the user would still need to extract the underlying associated error as an Error and then cast to the type they need. So I'm not sure adding a layer of indirection through our error type would help.

Copy link
Member

Choose a reason for hiding this comment

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

This is something we do in our error system built on top of AF @jshier and it's actually super helpful. This way, you can easily categorize the error types you care about. We'd probably need to introduce something like .user and .system cases into AFError that would wrap the underlying error. That would allow us to lock AFResult to AFError exclusively and would make it easier for clients, even though it makes it more difficult for us in AF.

Worth considering...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, thinking about it more, the doubly generic Result does give us more flexibility here. I just need to think out all of the ramifications of encapsulating all errors.

@ejensen
Copy link
Contributor

ejensen commented Mar 28, 2019

If we can't namespace the extensions I would just get rid of them and use our typealias to cover the API. There are few internal bits that will need to change but I'm not concerned about breaking Result usage. It's unfortunate but necessary. We could manually prefix the methods with af_ but I think I'd want to remove everything first and see what we want to keep.

I took a shot at namespacing the extensions under Result.af in commit db8b231
It doesn't use the AlamofireExtended protocol since the generics were fighting each other. But it results in the same signature & functionality.

Commit d38c4e9 updates the Documentation to show how internal-izing the extensions affect the public API.

@cnoon
Copy link
Member

cnoon commented Mar 28, 2019

Ah sorry @ejensen, I should have posted here that I've been working on that as well. The current gameplan is to move all the APIs on AFResult to internal, and remove all of them we can. I've been able to remove most of them since some are duplicates, but there are a few that are worth keeping for internal purposes.

The biggest question is whether we can to keep the value and error convenience properties. Right now we're leaning towards keeping them internal, and clients can easily add those extensions if they want them. Interested to get your thoughts on the matter as to which ones you'd like to still be public (probably under an af namespace).

@cnoon cnoon mentioned this pull request Mar 28, 2019
@cnoon
Copy link
Member

cnoon commented Mar 29, 2019

Thanks for all the leg-work here @AtomicCat and @ejensen! I've pulled all these changes into #2774 while maintaining both of your attributions. Let's continue the discussion on that PR. I'm going to close this one out for now.

@cnoon cnoon closed this Mar 29, 2019
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.

5 participants