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

Rewrite WordPressOrgRestApi #724

Merged
merged 17 commits into from
Feb 16, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ _None._

### Breaking Changes

_None._
- Rewrite `WordPressOrgRestApi` to support self hosted sites and WordPress.com sites. [#724]

### New Features

Expand Down
44 changes: 24 additions & 20 deletions WordPressKit.xcodeproj/project.pbxproj

Large diffs are not rendered by default.

153 changes: 0 additions & 153 deletions WordPressKit/Authenticator.swift

This file was deleted.

67 changes: 20 additions & 47 deletions WordPressKit/BlockEditorSettingsServiceRemote.swift
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import Foundation

public class BlockEditorSettingsServiceRemote {
let remoteAPI: WordPressRestApi
public init(remoteAPI: WordPressRestApi) {
let remoteAPI: WordPressOrgRestApi
public init(remoteAPI: WordPressOrgRestApi) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a minor nitpick / curiosity. I have a thing for type names using acronyms, like URL instead of Url or JSONDecoder instead of JsonDecoder.

How would this look like if it were WordPressORGRESTAPI? Too many uppercase letters? 😳

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd probably use Org instead of ORG. But this PR simply reuses the existing name 😄 .

self.remoteAPI = remoteAPI
}
}
Expand All @@ -11,30 +11,15 @@ public class BlockEditorSettingsServiceRemote {
public extension BlockEditorSettingsServiceRemote {
typealias EditorThemeCompletionHandler = (Swift.Result<RemoteEditorTheme?, Error>) -> Void

func fetchTheme(forSiteID siteID: Int?, _ completion: @escaping EditorThemeCompletionHandler) {
func fetchTheme(completion: @escaping EditorThemeCompletionHandler) {
let requestPath = "/wp/v2/themes"
let parameters: [String: AnyObject] = ["status": "active" as AnyObject]
let modifiedPath = remoteAPI.requestPath(fromOrgPath: requestPath, with: siteID)
remoteAPI.GET(modifiedPath, parameters: parameters) { [weak self] (result, _) in
guard let `self` = self else { return }
switch result {
case .success(let response):
self.processEditorThemeResponse(response) { editorTheme in
completion(.success(editorTheme))
}
case .failure(let error):
completion(.failure(error))
}
}
}

private func processEditorThemeResponse(_ response: Any, completion: (_ editorTheme: RemoteEditorTheme?) -> Void) {
guard let responseData = try? JSONSerialization.data(withJSONObject: response, options: []),
let editorThemes = try? JSONDecoder().decode([RemoteEditorTheme].self, from: responseData) else {
completion(nil)
return
let parameters = ["status": "active"]
Task { @MainActor in
let result = await self.remoteAPI.get(path: requestPath, parameters: parameters, type: [RemoteEditorTheme].self)
.map { $0.first }
.mapError { error -> Error in error }
completion(result)
}
completion(editorThemes.first)
}

}
Expand All @@ -43,30 +28,18 @@ public extension BlockEditorSettingsServiceRemote {
public extension BlockEditorSettingsServiceRemote {
typealias BlockEditorSettingsCompletionHandler = (Swift.Result<RemoteBlockEditorSettings?, Error>) -> Void

func fetchBlockEditorSettings(forSiteID siteID: Int?, _ completion: @escaping BlockEditorSettingsCompletionHandler) {
let requestPath = "/wp-block-editor/v1/settings"
let parameters: [String: AnyObject] = ["context": "mobile" as AnyObject]
let modifiedPath = remoteAPI.requestPath(fromOrgPath: requestPath, with: siteID)

remoteAPI.GET(modifiedPath, parameters: parameters) { [weak self] (result, _) in
guard let `self` = self else { return }
switch result {
case .success(let response):
self.processBlockEditorSettingsResponse(response) { blockEditorSettings in
completion(.success(blockEditorSettings))
func fetchBlockEditorSettings(completion: @escaping BlockEditorSettingsCompletionHandler) {
Task { @MainActor in
let result = await self.remoteAPI.get(path: "/wp-block-editor/v1/settings", parameters: ["context": "mobile"], type: RemoteBlockEditorSettings.self)
.map { settings -> RemoteBlockEditorSettings? in settings }
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a change for later on, but it seems like the only reason the RemoteBlockEditorSettings the API call returns is wrapped in an Optional here is because that's the type the "legacy" BlockEditorSettingsCompletionHandler expects?

If so, and since we're making breaking changes, I wonder whether we could make the type in the completion typealias non-optional. It would remove this step, and I suspect also simplify caller code by removing the need to deal with Result and Optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not just for fetchBlockEditorSettings's syntax level backward compatibility, but most importantly runtime behaviour backward compatibility.

/wp-block-editor/v1/settings endpoint returns a JSON that contains many properties. The fetchBlockEditorSettings function only cares about a subset of these properties. See the model type RemoteBlockEditorSettings, which contains mostly non-optional properties.

If the JSON response contains all the properties declared in the Swift type, this function of course should return a model instance.

However, if this API returns 200 with valid "block editor settings" JSON response, which does not contains all the non-optional proprieties declared in the RemoteBlockEditorSettings type, the function should return a nil, instead of a "decoding error".

That's the behaviour of the existing function. And the refactored implementation (mainly the line here and the code below that maps decoding error to a nil successful result) maintains the same runtime behaviour for backward compatibility.

.flatMapError { original in
if case let .unparsableResponse(response, _, underlyingError) = original, response?.statusCode == 200, underlyingError is DecodingError {
return .success(nil)
}
return .failure(original)
}
case .failure(let error):
completion(.failure(error))
}
}
}

private func processBlockEditorSettingsResponse(_ response: Any, completion: (_ editorTheme: RemoteBlockEditorSettings?) -> Void) {
guard let responseData = try? JSONSerialization.data(withJSONObject: response, options: []),
let blockEditorSettings = try? JSONDecoder().decode(RemoteBlockEditorSettings.self, from: responseData) else {
completion(nil)
return
.mapError { error -> Error in error }
completion(result)
}
completion(blockEditorSettings)
}
}
5 changes: 3 additions & 2 deletions WordPressKit/HTTPClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,11 @@ extension WordPressAPIResult {
}

func decodeSuccess<NewSuccess: Decodable, E: LocalizedError>(
_ decoder: JSONDecoder = JSONDecoder()
_ decoder: JSONDecoder = JSONDecoder(),
type: NewSuccess.Type = NewSuccess.self
) -> WordPressAPIResult<NewSuccess, E> where Success == HTTPAPIResponse<Data>, Failure == WordPressAPIError<E> {
mapSuccess {
try decoder.decode(NewSuccess.self, from: $0.body)
try decoder.decode(type, from: $0.body)
}
}

Expand Down
16 changes: 16 additions & 0 deletions WordPressKit/HTTPProtocolHelpers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,22 @@ extension HTTPURLResponse {
return Self.value(ofParameter: parameterName, inHeaderValue: headerValue, stripQuotes: stripQuotes)
}

func value(forHTTPHeaderField field: String, withoutParameters: Bool) -> String? {
guard withoutParameters else {
return value(forHTTPHeaderField: field)
}

guard let headerValue = value(forHTTPHeaderField: field) else {
return nil
}

guard let firstSemicolon = headerValue.firstIndex(of: ";") else {
return headerValue
}

return String(headerValue[headerValue.startIndex..<firstSemicolon])
}

static func value(ofParameter parameterName: String, inHeaderValue headerValue: String, stripQuotes: Bool = true) -> String? {
// Find location of '<parameter>=' string in the header.
guard let location = headerValue.range(of: parameterName + "=", options: .caseInsensitive) else {
Expand Down
17 changes: 17 additions & 0 deletions WordPressKit/HTTPRequestBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,23 @@ final class HTTPRequestBuilder {
return self
}

/// Append path and query to the original URL.
///
/// Some may call API client using a string that contains path and query, like `api.get("post?id=1")`. This function
/// can be used to support those use cases.
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting nitpick

Suggested change
/// Some may call API client using a string that contains path and query, like `api.get("post?id=1")`. This function
/// can be used to support those use cases.
/// Some may call API client using a string that contains path and query, like `api.get("post?id=1")`.
/// This function can be used to support those use cases.

func appendURLString(_ string: String) -> Self {
let urlString = Self.join("https://w.org", string)
guard let url = URL(string: urlString),
let urlComponents = URLComponents(url: url, resolvingAgainstBaseURL: true)
else {
assertionFailure("Illegal URL string: \(string)")
return self
}

return append(percentEncodedPath: urlComponents.percentEncodedPath)
.append(query: urlComponents.queryItems ?? [])
}

func header(name: String, value: String?) -> Self {
headers[name] = value
return self
Expand Down
Loading