Define skeleton for new WordPressData module by mokagio · Pull Request #24153 · wordpress-mobile/WordPress-iOS

mokagio · 2025-03-06T04:32:11Z

Internal ref pbArwn-7b1-p2

Defines an Objective-C + Swift combo package "WordPressData" where we'll progressively house all of the model definition, storage, and services
Move CoreDataStack(Swift) in WordPressData

Testing

See green CI as a proof that the compilation is successful
Try out the prototype builds, I guess

Next steps

Move the xcdatamodeld folder into the package. Also check out how Tracks does it
- Init the Core Data stack using the model from the package
Move all the models (I have a feeling they need to be moved in bulk, but I might be wrong of course) to the package
Move types heavily related with Core Data, such as TaggedManagedObjectID into the package

Looking at the code, I noticed two things that we need to take a decision on:

Some of the code uses CocoaLumberjack. Ditch the logs, add it as a dependency, or implement an abstraction and injection system like we did for WordPressAuthenticator?
Some of the code uses WPException. Make it into its own package to import it in WordPressData and in the other code using WPException, or expose it through WordPressData and make those types that need it import WordPressData.

My thoughts in the details. What do you think @crazytonyli @kean ?

Some of the code uses CocoaLumberjack. As far as I know we don't use those logs much. If that's correct, let's keep things simple and remove the dependency.
Some of the code uses WPException. Packages are cheap to add thanks to the current setup. Let's keep things clean and make a micro-package for WPException. It also keeps things clearer because importing a model/data package only to use an exception seems confusing to me.

Regression Notes

Potential unintended areas of impact — Anything that has to do with data storage in the app...
What I did to test those areas of impact (or what existing automated tests I relied on) — At this time, only a quick smoke tests of the prototype builds, plus relied on the existing unit tests
What automated tests I added (or what prevented me from doing so) — None, as I only moved code at this time.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes. — N.A.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing checklist:

dangermattic · 2025-03-06T04:32:48Z

	2 Warnings
⚠️	`Modules/Package.swift` was changed without updating its corresponding `Package.resolved`. Please resolve the Swift packages as appropriate to your project setup (e.g. in Xcode or by running `swift package resolve`).
⚠️	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 · 2025-03-06T05:30:34Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr24153-758008b
	Version	25.8
	Bundle ID	com.jetpack.alpha
	Commit	`758008b`
	App Center Build	jetpack-installable-builds #10635

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2025-03-06T05:32:17Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr24153-758008b
	Version	25.8
	Bundle ID	org.wordpress.alpha
	Commit	`758008b`
	App Center Build	WPiOS - One-Offs #11604

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

mokagio · 2025-03-06T11:00:40Z

-import UIKit
+import AsyncImageKit
 import CoreData
+import UIKit
+import WordPressData
 import WordPressShared
-import AsyncImageKit


In hindsight, sorting the imports alphabetically in the context of this PR makes the diff harder to read. Apologies.

kean · 2025-03-06T13:43:03Z

Ditch the logs, add it as a dependency, or implement an abstraction and injection system like we did for WordPressAuthenticator?

I would add CocoaLumberjack as a dependency. WordPressAuthenticator has an abstraction because it used to be in a separate repo and used by another app (s). If we want an abstraction, I suggest https://github.com/apple/swift-log but not in this project's scope.

Some of the code uses WPException. Make it into its own package to import it in WordPressData and in the other code using WPException, or expose it through WordPressData and make those types that need it import WordPressData.

I suggest moving it to WordPressSharedObjC. This a small package with helper types like this. I'd say it's a no-goal to introduce too many new packages as it can make it harder to maintain and could impact compile time in a negative way. @_exported in an umbrella framework is, of course, also always an option.

As far as I know we don't use those logs much.

Your call. If it's not logging anything you consider useful, I'd also remove it.

kean · 2025-03-06T17:05:23Z

Btw, I really like the name "WordPressData" – rhymes with "CoreData".

mokagio · 2025-03-06T21:50:15Z

Btw, I really like the name "WordPressData" – rhymes with "CoreData".

Glad you like it 😄

@kean

See conversation with @kean at #24153 (comment) This will become useful as some of the code using `WPException` will move away from the main target and into one or more modules.

@kean

See conversation with @kean at #24153 (comment) This will become useful as some of the code using `WPException` will move away from the main target and into one or more modules.

mokagio · 2025-03-06T23:00:48Z

I suggest moving it [WPException] to WordPressSharedObjC

Cool. Done via #24157

kean

LGTM. Smoke tested.

See how https://buildkite.com/automattic/wordpress-ios/builds/25994 failed: ``` error: Build input file cannot be found: '/opt/ci/builds/builder/automattic/wordpress-ios/DerivedData/Build/Intermediates.noindex/ArchiveIntermediates/WordPress Alpha/BuildProductsPath/Release-Alpha-iphoneos/WordPressDataObjC.o'. Did you forget to declare this file as an output of a script phase or custom build rule which produces it? (in target 'WordPress' from project 'WordPress') ```

Not sure how it was working before — and in CI too, see https://buildkite.com/automattic/wordpress-ios/builds/25995 My only guess is that the file became available in the DerivedData somehow because the Swift package importing it was built before the app targets? Odd...

It was needed just because there was no file in the package yet, but we've addressed it whenw e moved `CoreDataService` in there.

@kean

See conversation with @kean at #24153 (comment) This will become useful as some of the code using `WPException` will move away from the main target and into one or more modules.

…ta-skeleton

mokagio force-pushed the mokagio/wordpress-data-skeleton branch from 2131a51 to 325bf00 Compare March 6, 2025 05:03

mokagio mentioned this pull request Mar 6, 2025

Remove an unused Coordinate model #24154

Merged

mokagio force-pushed the mokagio/wordpress-data-skeleton branch from 5252c48 to 518924d Compare March 6, 2025 10:12

mokagio marked this pull request as ready for review March 6, 2025 10:26

mokagio requested review from crazytonyli and kean and removed request for kean March 6, 2025 10:26

mokagio added the Core Data Issues related to Core Data label Mar 6, 2025

mokagio added this to the 25.9 milestone Mar 6, 2025

mokagio force-pushed the mokagio/wordpress-data-skeleton branch 3 times, most recently from bb53b69 to c14928d Compare March 6, 2025 10:58

mokagio commented Mar 6, 2025

View reviewed changes

mokagio mentioned this pull request Mar 6, 2025

Move WPException to WordPressSharedObjC #24157

Merged

mokagio changed the title ~~Defined skeleton for new WordPressData module~~ Define skeleton for new WordPressData module Mar 6, 2025

kean approved these changes Mar 7, 2025

View reviewed changes

mokagio added 4 commits March 7, 2025 13:00

Move CoreDataStack to new WordPressData module

62a7eed

Add @_exported import for WordPressDataObjC in WordPressData

2985a0b

mokagio added 2 commits March 7, 2025 13:00

Move CoreDataService to WordPressData

f211909

Remove Dummy from WordPressDataObjC — No longer needed

2d08677

It was needed just because there was no file in the package yet, but we've addressed it whenw e moved `CoreDataService` in there.

mokagio force-pushed the mokagio/wordpress-data-skeleton branch from c14928d to 2d08677 Compare March 7, 2025 02:00

Merge remote-tracking branch 'origin/trunk' into mokagio/wordpress-da…

758008b

…ta-skeleton

mokagio enabled auto-merge March 7, 2025 06:56

mokagio mentioned this pull request Mar 7, 2025

Migrate Core Data schema, models, and manager to dedicated WordPressData module. #24165

Open

mokagio added this pull request to the merge queue Mar 7, 2025

Merged via the queue into trunk with commit 9e4d06a Mar 7, 2025

mokagio deleted the mokagio/wordpress-data-skeleton branch March 7, 2025 07:52

mokagio mentioned this pull request Mar 7, 2025

Move +classNameWithoutNamespaces to WordPressSharedObjC #24164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Define skeleton for new WordPressData module - #24153

Define skeleton for new WordPressData module#24153
mokagio merged 7 commits into
trunkfrom
mokagio/wordpress-data-skeleton

mokagio commented Mar 6, 2025 •

edited

Loading

Uh oh!

dangermattic commented Mar 6, 2025 •

edited

Loading

Uh oh!

wpmobilebot commented Mar 6, 2025 •

edited

Loading

Uh oh!

wpmobilebot commented Mar 6, 2025 •

edited

Loading

Uh oh!

mokagio Mar 6, 2025

Uh oh!

kean commented Mar 6, 2025 •

edited

Loading

Uh oh!

kean commented Mar 6, 2025

Uh oh!

mokagio commented Mar 6, 2025

Uh oh!

mokagio commented Mar 6, 2025

Uh oh!

kean left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

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

Uh oh!

Regression Notes

Uh oh!

dangermattic commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mokagio Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

kean commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kean commented Mar 6, 2025

Uh oh!

mokagio commented Mar 6, 2025

Uh oh!

mokagio commented Mar 6, 2025

Uh oh!

kean left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mokagio commented Mar 6, 2025 •

edited

Loading

dangermattic commented Mar 6, 2025 •

edited

Loading

wpmobilebot commented Mar 6, 2025 •

edited

Loading

wpmobilebot commented Mar 6, 2025 •

edited

Loading

kean commented Mar 6, 2025 •

edited

Loading