Skip to content

Move ReachabilityUtils to WordPressShared - #24267

Merged
mokagio merged 15 commits into
trunkfrom
mokagio/reachability-helper
Mar 26, 2025
Merged

Move ReachabilityUtils to WordPressShared#24267
mokagio merged 15 commits into
trunkfrom
mokagio/reachability-helper

Conversation

@mokagio

@mokagio mokagio commented Mar 20, 2025

Copy link
Copy Markdown
Contributor

Problem

As part of #24165, there are certain pieces that need to be moved which depend on ReachabilityUtils an helper object that is also shared with the apps targets.

Solution

  • Rewrites ReachabilityUtils and the underlying ReachabilityAlert in Swift
  • Moves them to WordPressShared

I considered moving it to a dedicated package because I thought it used the actual Reachability library, but that turned out not to be the case.

I considered ditching this utility that does not support injection or testing, but decided against it to focus on #24165 instead.

@mokagio mokagio added this to the 25.9 milestone Mar 20, 2025
@dangermattic

dangermattic commented Mar 20, 2025

Copy link
Copy Markdown
Collaborator
4 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.
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ This PR is assigned to the milestone 25.9. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

@mokagio
mokagio force-pushed the mokagio/reachability-helper branch 2 times, most recently from 3fef4e5 to 3d7c75b Compare March 21, 2025 05:18
@wpmobilebot

wpmobilebot commented Mar 21, 2025

Copy link
Copy Markdown
Contributor
Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr24267-7faaf9b
Version25.8
Bundle IDcom.jetpack.alpha
Commit7faaf9b
App Center Buildjetpack-installable-builds #10863
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot

wpmobilebot commented Mar 21, 2025

Copy link
Copy Markdown
Contributor
WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr24267-7faaf9b
Version25.8
Bundle IDorg.wordpress.alpha
Commit7faaf9b
App Center BuildWPiOS - One-Offs #11840
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Comment thread Modules/Sources/WordPressShared/Utility/UIAlertController+Presentation.swift Outdated
@mokagio
mokagio marked this pull request as ready for review March 21, 2025 06:57
@mokagio
mokagio requested review from crazytonyli and kean and removed request for kean March 21, 2025 07:01
@mokagio mokagio changed the title Move ReachabilityUtils to dedicated framework Move ReachabilityUtils to WordPressShared Mar 21, 2025
@mokagio
mokagio force-pushed the mokagio/reachability-helper branch from b8335c4 to 91c8d33 Compare March 21, 2025 07:02
Comment thread Modules/Sources/WordPressShared/Reachability/ReachabilityUtils.swift Outdated
@mokagio

mokagio commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

I feel like you could extract the AppDelegate.configureReachability() part to here too, so that you don't have to indirectly rely on the app delegate instance.

Thanks @crazytonyli I'll have a look 👀

@mokagio
mokagio force-pushed the mokagio/reachability-helper branch from b72ff41 to 4fe98c1 Compare March 24, 2025 08:30
[self preloadComments];
[self preloadMetadata];
[self preloadDomains];
if ([ReachabilityUtils.internetReachability isReachableViaWiFi] == false) {

@mokagio mokagio Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Notice this accesses Reachability. I think we could work on this further and hide the library altogether under ReachabilityUtils. That is, this could call a method on ReachabilityUtils instead—law of Demeter

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If this makes sense to you, I'll create an issue to address this once #24165 is "done"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The Reachability library probably can be replaced with NWPath, but that's perhaps a story for another day.


WordPressAppDelegate *appDelegate = [WordPressAppDelegate shared];
[[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(reachabilityChanged:) name:kReachabilityChangedNotification object:appDelegate.internetReachability];
[[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(reachabilityChanged:) name:kReachabilityChangedNotification object:ReachabilityUtils.internetReachability];

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I converted the code here to pass the same Reachability instance, but I think we could omit it.

    [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(reachabilityChanged:) name:kReachabilityChangedNotification];

From the docs:

The object that sends notifications to the observer. Specify a notification sender to deliver only notifications from this sender.

When nil, the notification center doesn’t use sender names as criteria for delivery.

The handler method does

- (void)reachabilityChanged:(NSNotification *)notification
{
    Reachability *reachability = notification.object;
    if (reachability.isReachable) {
        [self initStats];
    }
}

We could replace add the info about connectivity as userInfo in the Notification and then we'd be good.

Comment on lines +3003 to +3006
24CE68B42CD3375300C7B37D /* Exceptions for "Classes" folder in "WordPressUITests" target */ = {
24CE68AF2CD3375300C7B37D /* Exceptions for "Classes" folder in "WordPressShareExtension" target */ = {
isa = PBXFileSystemSynchronizedBuildFileExceptionSet;
membershipExceptions = (
"Extensions/UIApplication+MainWindow.swift",
"Utility/App Configuration/AppConfiguration.swift",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Mmm... this and the rest of the diff look like a bad rebase

@mokagio
mokagio force-pushed the mokagio/reachability-helper branch 4 times, most recently from d94ca6d to ba440e7 Compare March 25, 2025 01:49
@mokagio
mokagio force-pushed the mokagio/reachability-helper branch from ba440e7 to 7faaf9b Compare March 26, 2025 00:10
@mokagio
mokagio added this pull request to the merge queue Mar 26, 2025
Merged via the queue into trunk with commit a7d92f7 Mar 26, 2025
@mokagio
mokagio deleted the mokagio/reachability-helper branch March 26, 2025 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants