Skip to content

Conversation

crazytonyli
Copy link
Contributor

Description

Note

Review commit by commit. There are not many actual code changes.

wordpress-mobile/WordPressKit-iOS#845 sorted out the dependencies among the Objective-C and Swift code in WordPressKit. We ended up having four modules:

graph LR
      A[WordPressKit]:::swift --> B[WordPressKitObjC]:::objc
      B --> C[WordPressKitModels]:::swift
      C --> D[WordPressKitObjCUtils]:::objc

      classDef swift fill:#FFA500,stroke:#333,stroke-width:2px,color:#000
      classDef objc fill:#4169E1,stroke:#333,stroke-width:2px,color:#fff
Loading

The WordPressKitObjC module contains "remote service" types that make API calls and return model instances. Some of those model types are defined in Swift code. For example, -[BlogServiceRemoteREST syncBlogSettingsWithSuccess:failure:] returns RemoteBlogSettings, which is in Swift.

One of the problems I encounter during this integration is that the Objective-C API like the syncBlogSettings is not available to the app's Swift code, because the compiler somehow does not see the concrete type of RemoteBlogSettings (which is forward-declared in the BlogServiceRemoteREST.h header file). Here is a discussion about this issue on the Swift forum.

In addition to the OTHER_SWIFT_FLAGS solution suggested in the above discussion, we also need to make sure the WordPressKit module is imported into the Xcode target's bridging header.

However, we can't provide either of these settings to Swift Package Manager targets. And as a result of that, this getLikesForPostID call in the JetpackStats module fails to compile. I ended up having to translate the getLikesForPostID Objective-C code into Swift. Luckily, there is only one such case, so we don't need to do a whole lot of unnecessary translation.

Next step

First, review the WordPressKit changes in wordpress-mobile/WordPressKit-iOS#846. If both PRs look good, I'll merge this PR and close the WordPressKit one.

Next, I'll create a new PR to copy the WordPressKit code into this repo, which we can merge without reviewing.

After that, I'll create a test target in this Xcode project to run the WordPressKit unit tests. It needs to be in Xcode, instead of Modules/Package.swift, because we will likely need to provide the OTHER_SWIFT_FLAGS build setting and the bridging header.

@crazytonyli crazytonyli added this to the 26.2 milestone Aug 28, 2025
@crazytonyli crazytonyli requested a review from kean August 28, 2025 01:33
@dangermattic
Copy link
Collaborator

1 Warning
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Aug 28, 2025

App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
ConfigurationRelease-Alpha
Build Number28694
VersionPR #24771
Bundle IDcom.jetpack.alpha
Commit0d83ce8
Installation URL5ikedsaa16l0g
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Aug 28, 2025

App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
ConfigurationRelease-Alpha
Build Number28694
VersionPR #24771
Bundle IDorg.wordpress.alpha
Commit0d83ce8
Installation URL0agojf7cbrf8g
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@kean kean modified the milestones: 26.2, 26.3 Aug 28, 2025
Copy link
Contributor

@kean kean left a comment

Choose a reason for hiding this comment

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

In addition to the OTHER_SWIFT_FLAGS solution suggested in the above discussion, we also need to make sure the WordPressKit module is imported into the Xcode target's bridging header.

Interesting. I didn't know about that limitation. It looks like we'll have to eliminate WordPressKitObjC and rewrite it to Swift or at least the parts the depend on Swift models (probably a better option).

@@ -1,5 +1,6 @@
import Foundation
import WordPressShared
@preconcurrency import WordPressKitModels
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) WordPressKit should probably re-export WordPressKitModels for convenience

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are already re-exported. I have removed this import, and a couple of others too, in a8146d6.

@crazytonyli crazytonyli enabled auto-merge August 31, 2025 22:17
Copy link

@crazytonyli crazytonyli added this pull request to the merge queue Aug 31, 2025
Merged via the queue into trunk with commit ea208f1 Aug 31, 2025
30 of 32 checks passed
@crazytonyli crazytonyli deleted the wordpresskit-spm branch August 31, 2025 23:06
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.

4 participants