SPM Prep – Add `@protocol` defining REST GET and POST methods by mokagio · Pull Request #760 · wordpress-mobile/WordPressKit-iOS

mokagio · 2024-03-21T02:04:52Z

This PR is meant to get early feedback on the design direction for the abstraction layer required for SPM support.

It:

Defines a minimal abstraction layer via Objective-C protocol tentatively named WordPressComRESTAPIInterfacing
Makes WordPressComRestApi conform to the protocol
Updates a few existing requests to use the abstraction, to ensure the various pieces are wired together correctly.

I'm aware of the CocoaPods validation failure. Working on fixing it by updating the paths definitions in the podspec.

Testing Details

CI is enough.

Next up

Basically, cherry-pick the work from #738 but in a neat and reviewable order:

Move files into Sources/ and Tests/
Introduce Bundle helper that differentiates between SPM and CocoaPods installations
Isolate core API objects in dedicated package

See also, #756 and #758 which is related to the SPM work but independent from this one.

Please check here if your pull request includes additional test coverage. — N.A.
I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary. — N.A.

mokagio · 2024-03-21T02:08:34Z

Sources/APIInterface/include/WordPressComRESTAPIInterfacing.h

@@ -0,0 +1,15 @@
+@import Foundation;
+
+@protocol WordPressComRESTAPIInterfacing


Two notes on the naming:

I used "REST" and "API", instead of "Rest" and "Api" because they are acronyms and it feels more correct to have them all uppercase (when not used as the first word of a method).

I used "Interfacing" to align with Swift naming conventions. In that context, an object conforming to this protocol should "interface with the WordPress.com REST API" which is exactly what WordPressComRestApi does.

I'm a bit concerned by the ambiguity between "interfacing" as a verb and "interface" as a noun often used in programming languages to define something similar to an Objective-C or Swift protocol. Any suggestion for an alternative name is welcome.

mokagio · 2024-03-21T02:10:02Z

Sources/APIInterface/include/WordPressComRESTAPIInterfacing.h

+- (void)get:(NSString * _Nonnull)URLString
+ parameters:(NSDictionary<NSString *, NSObject *> * _Nullable)parameters
+    success:(void (^ _Nonnull)(id _Nonnull, NSHTTPURLResponse * _Nullable))success
+    failure:(void (^ _Nonnull)(NSError * _Nonnull, NSHTTPURLResponse * _Nullable))failure;
+
+- (void)post:(NSString * _Nonnull)URLString


I decided not to uppercase GET and POST like elsewhere in the code because of the naming convention of starting methods names in lowercase.

Interestingly, even if uppercase, the conversion from Objective-C to Swift results in the word being lowercased in the generated Swift version.

Also interesting, Xcode suggested the method signature. I think it's a new Xcode 15.2/15.3 feature? I was pleasantly surprised. Definitely saved me some time in figuring out the block signatures.

mokagio · 2024-03-21T02:13:00Z

WordPressKit.xcodeproj/project.pbxproj

+		3FFCC0532BABC75F0051D229 /* Sources */ = {
+			isa = PBXGroup;
+			children = (
+				3FFCC0542BABC7680051D229 /* APIInterface */,
+			);
+			path = Sources;
+			sourceTree = "<group>";
+		};
+		3FFCC0542BABC7680051D229 /* APIInterface */ = {
+			isa = PBXGroup;
+			children = (
+				3FFCC0562BABC7D20051D229 /* include */,
+			);
+			path = APIInterface;
+			sourceTree = "<group>";
+		};
+		3FFCC0562BABC7D20051D229 /* include */ = {
+			isa = PBXGroup;
+			children = (
+				3FFCC0552BABC78B0051D229 /* WordPressComRESTAPIInterfacing.h */,
+			);
+			path = include;
+			sourceTree = "<group>";
+		};


I took the chance of adding this new file to setup the Sources/ SPM structure.

After this lands, I'll make a dedicated PR that moves all the files in Sources/WordPressKit and a new Tests/WordPressKitTests.

Once that happens, I'll follow up with another PR that extracts WordPressComRestApi and the related types in something I'm tentatively naming "CoreAPI", as per #738

mokagio · 2024-03-21T02:14:48Z

WordPressKit/ServiceRemoteWordPressComREST.h

+ *  @brief      The interface to the WordPress.com API to use for performing REST requests.
+ *              This is meant to gradually replace `wordPressComRestApi`.
+ */
+@property (nonatomic, strong, readonly) id<WordPressComRESTAPIInterfacing> wordPressComRESTAPI;


The aim of this PR is to get feedback on the direction. As such, I haven't gone through the considerable effort of replacing all usages of the concrete WordPressComRestApi *wordPressComRestApi with the abstracted id<WordPressComRESTAPIInterfacing> wordPressComRESTAPI.

Assuming the direction I envisioned resonates, that'll be my next step.

mokagio · 2024-03-21T02:17:15Z

WordPressKit/ServiceRemoteWordPressComREST.m

    self = [super init];
    if (self) {
        _wordPressComRestApi = wordPressComRestApi;
+        _wordPressComRESTAPI = wordPressComRestApi;


Once all the concrete WordPressComRestApi usages will have been replaced, this init will take an id<WordPressComRESTAPIInterfacing> input, effectively decoupling ServiceRemoteWordPressComREST (Objective-C) from WordPressComRestApi (Swift) via WordPressComRESTAPIInterfacing (Objective-C).

This is the model implemented in https://github.com/Automattic/spm-abstraction-layer-demo which should allow us to organize the code in a Swift package that Xcode can compile.

Use it in one call withing `AccountServiceRemoteREST`. The tests verify it works as expected.

Used for all POST requests in `AccountServiceRemoteREST`

The SPM-compatible folder structure was giving me trouble when trying to validate the `podspec`. At first, I thought it was simply because `WordPressComRESTAPIInterfacing.h` was not in a path known to CocoaPods. But even after updating the `podspec` to: ```diff @@ -18,9 +18,10 @@ Pod::Spec.new do |s| s.swift_version = '5.0' s.source = { git: 'https://github.com/wordpress-mobile/WordPressKit-iOS.git', tag: s.version.to_s } - s.source_files = 'WordPressKit/**/*.{h,m,swift}' + + s.source_files = 'WordPressKit/**/*.{h,m,swift}', 'Sources/**/*.{h,m,swift}' + s.public_header_files = 'Sources/**/include/*.h', 'WordPressKit/**/.h', 'WordPressKit/WordPressKit.h' s.private_header_files = 'WordPressKit/Private/*.h' - s.header_dir = 'WordPressKit' s.dependency 'NSObject-SafeExpectations', '~> 0.0.4' s.dependency 'wpxmlrpc', '~> 0.10' ``` I still got errors: ``` - ERROR | xcodebuild: /var/folders/dq/cdqxvx3s5ps75564rpmb_dc00000gn/T/CocoaPods-Lint-20240321-63072-nntpgw-WordPressKit/DerivedData/App/Build/Products/Release-iphonesimulator/WordPressKit/WordPressKit.framework/Headers/WordPressKit.h:10:9: error: 'WordPressKit/ServiceRemoteWordPressComREST.h' file not found ``` In the proof of concept from #738, the SPM folder structure is compatible with CocoaPods. As such, I decided to defer moving the files to when I can do it in one go.

mokagio commented Mar 21, 2024

View reviewed changes

mokagio requested a review from crazytonyli March 21, 2024 02:17

crazytonyli approved these changes Mar 21, 2024

View reviewed changes

mokagio added 5 commits March 22, 2024 14:00

Define WordPressComRESTAPIInterfacing abstraction with GET method

fae09dd

Use it in one call withing `AccountServiceRemoteREST`. The tests verify it works as expected.

Make all GET requests in AccountServiceRemoteREST via new interface

73d51e7

Add POST method to WordPressComRESTAPIInterfacing

a7415a0

Used for all POST requests in `AccountServiceRemoteREST`

Add note about wordPressComRestApi vs wordPressComRESTAPI

de1c24c

mokagio force-pushed the mokagio/api-abstraction-layer branch from 779ec14 to b65746d Compare March 22, 2024 03:02

mokagio enabled auto-merge March 22, 2024 03:02

mokagio merged commit 13289c3 into trunk Mar 22, 2024

mokagio deleted the mokagio/api-abstraction-layer branch March 22, 2024 03:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPM Prep – Add `@protocol` defining REST GET and POST methods#760

SPM Prep – Add `@protocol` defining REST GET and POST methods#760
mokagio merged 5 commits intotrunkfrom
mokagio/api-abstraction-layer

mokagio commented Mar 21, 2024 •

edited

Loading

Uh oh!

mokagio Mar 21, 2024

Uh oh!

mokagio Mar 21, 2024

Uh oh!

mokagio Mar 21, 2024 •

edited

Loading

Uh oh!

mokagio Mar 21, 2024

Uh oh!

mokagio Mar 21, 2024

Uh oh!

mokagio Mar 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,15 @@
		@import Foundation;

		@protocol WordPressComRESTAPIInterfacing

Conversation

mokagio commented Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing Details

Next up

Uh oh!

mokagio Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

mokagio Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

mokagio Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

mokagio Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

mokagio Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mokagio commented Mar 21, 2024 •

edited

Loading

mokagio Mar 21, 2024 •

edited

Loading