Use URLSession to send WP.com REST API requests #720
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
crazytonyli
force-pushed
the
wordpress-com-rest-api-error-refactor
branch
2 times, most recently
from
3dd50a3 to
e30ba27
Compare
2 tasks
mokagio
approved these changes
Feb 5, 2024
| @@ -612,6 +639,38 @@ open class WordPressComRestApi: NSObject { | |||
| } | |||
| } | |||
|
|
|||
| public func upload( | |||
| _ URLString: String, | |||
There was a problem hiding this comment.
Nitpick. Value names should start with lowercase letters.
Suggested change
| _ URLString: String, | |
| _ urlString: String, |
There was a problem hiding this comment.
I have updated this part in a3c19a9. which probably made it worse, in terms of the name... I used URLString because that's the term used throughout this class...
| completion( | ||
| result | ||
| .map { ($0.body, $0.response) } | ||
| .mapError { error -> Error in error } |
There was a problem hiding this comment.
It wasn't obvious to me why this mapError was necessary.
What to you think of adding a line such as
Suggested change
| .mapError { error -> Error in error } | |
| // The completion expects a result with any Error. | |
| // We need to "downcast" the WordPressComRestApiEndpointError error in the APIResult. | |
| .mapError { error -> Error in error } |
Also, using $0 would work, too. But, having the explicit function signature with Error might be clearer for the reader in this unusual scenario.
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR updates the WP.com REST API client to send HTTP requests using URLSession, when
useURLSessionis set to true (it's false by default).Note
I'd recommend reviewing this PR commit-by-commit.
Testing Details
The unit test suite passes with
useURLSessionset to ture. I had to updated one test case to make it pass, because the new URLSession API does not return AFError.In theory, all the new code in this PR are dead code at the moment. I will update the apps to enable
useURLSessionas in-development feature flag, and we can see how the new implementation work out in the app.wordpress-mobile/WordPress-iOS#22540 can be used to get an app build which enables
useURLSessionby default.CHANGELOG.mdif necessary.