Use URLSession in WordPressOAuthClient - #660
Conversation
mokagio
left a comment
mokagio
left a comment
There was a problem hiding this comment.
Just a few comments... I didn't finish the review but wanted to avoid losing these.
|
|
||
| func url(base: String) -> URL { | ||
| return URL(string: self.rawValue, relativeTo: URL(string: base))! | ||
| func url(base: URL) -> URL { |
There was a problem hiding this comment.
👍 for requiring base to be a URL and pushing the responsibility to create good URLs to the callers.
IMHO, in the ideal world, we'd only have one point in the whole app / library where we go from String to URL. The rest of the code deals with "valid" instances, without having to worry about validation.
|
|
||
| private let oauth2SessionManager: SessionManager = { | ||
| return WordPressComOAuthClient.sessionManager() | ||
| // Question: Is it necessary to use these many URLSession instances? |
There was a problem hiding this comment.
What do you think of making this a TODO or FIXME just so tooling can pick it up?
There was a problem hiding this comment.
Or... have you discovered an answer since writing that comment with the extra work you've done on this library?
There was a problem hiding this comment.
I didn't use TODO or FIXME because I'm not sure it needs a change.
But the pattern sure raises a question, at least to me. Because it's pretty rare to see code that uses different URLSession instances to send requests to endpoints that are in the same area (in this case, authentication). But considering the "endpoints" here are all php pages, rather than the common REST style API endpoints, maybe it wants to avoid sharing cookies among those API calls?
| self.wordPressComBaseUrl = wordPressComBaseUrl | ||
| self.wordPressComApiBaseUrl = wordPressComApiBaseUrl | ||
| self.wordPressComBaseUrl = URL(string: wordPressComBaseUrl)! | ||
| self.wordPressComApiBaseUrl = URL(string: wordPressComApiBaseUrl)! |
There was a problem hiding this comment.
If we plan on more breaking changes, we could also slot one in to require those String parameters to be URLs.
|
Need to rebase this PR after #668 is merged. |
| public enum AuthenticationResult { | ||
| case authenticated(token: String) | ||
| case needsMultiFactor(userID: Int, nonceInfo: SocialLogin2FANonceInfo) | ||
| } |
|
|
||
| private let oauth2SessionManager: SessionManager = { | ||
| return WordPressComOAuthClient.sessionManager() | ||
| // Question: Is it necessary to use these many URLSession instances? |
There was a problem hiding this comment.
Or... have you discovered an answer since writing that comment with the extra work you've done on this library?
| }) | ||
| return .needsMultiFactor(userID: userID, nonceInfo: nonceInfo) | ||
| }, | ||
| failure: Self.processError(_:) |
There was a problem hiding this comment.
Nitpick: I find calling Self. clunky, but that might just be me.
Just for fun, we could
- Make
processErroraprivatefree function; or - It could be a method on
HTTPAPIResponse<Data>; or - An
init?onAuthenticationFailure
But... 🤷♂️ This is internal code that's only used here.
| func tokenRequestBuilder() -> HTTPRequestBuilder { | ||
| HTTPRequestBuilder(url: wordPressComApiBaseUrl) | ||
| .method(.post) | ||
| .append(path: "/oauth2/token") | ||
| } |
There was a problem hiding this comment.
Love to see these builders in action and to DRY things.
1e43fe9 to
ce168a2
Compare
a243bb8 to
a83f361
Compare
2 participants
Description
This PR replaces some usages of Alamofire in
WordPressOAuthClient, withURLSession. There will be more similar PRs later for other usages.I noticed there are many
Alamofire.SessionManagerinstances are used. I don't understand why, but at the same time I'm too afraid to change it. For now, I'll follow the existing pattern and continue using manyURLSessioninstances too.Testing Details
The refactored features are: login with WP.com username password and request code via SMS. You can use wordpress-mobile/WordPress-iOS#22215 to test this change. Here are some test scenarios I can think of:
CHANGELOG.mdif necessary.