[Tech Debt] Change `TARGET_OS_IPHONE` macros in code

[mindgraffiti]

Xcode 10 and Swift 4.2 no longer recognize the TARGET_OS_IPHONE macro, which was deprecated in Swift 3 and removed in 4.2. The side effect of this is that Xcode is now executing code that was not meant for simulators.

The code need reviewed and all TARGET_OS_IPHONE macros replaced. I don't have a specific recommendation because this project may also need updated to target iOS 9.2 instead of 9.0 for full compatibility with the new macros.

Ref. https://github.com/Automattic/Automattic-Tracks-iOS/pull/78 Ref. https://github.com/woocommerce/woocommerce-ios/pull/475

[jkmassel]

Are we certain that TARGET_OS_IPHONE is deprecated?

Based on this: https://stackoverflow.com/a/37891230/496295, and /usr/include/TargetConditionals.h, it seems that TARGET_IPHONE_SIMULATOR is deprecated in favour of TARGET_OS_SIMULATOR, but I can't find a reference to the deprecation of TARGET_OS_IPHONE.

That said, if you're using Swift, the #if TARGET_OS_IPHONE syntax seems nicer, but if I'm not mistaken, isn't it still just referencing the same global constant?

@mindgraffiti – do you know what we should be migrating to instead? :)

[mindgraffiti]

@jkmassel deprecated is the incorrect word, you are correct +1

The goal is to take the code that is isolated by TARGET_OS_IPHONE, and make it skip execution for simulators. So my suggestion would be to use TARGET_OS_IPHONE && !TARGET_OS_SIMULATOR in this project.

Thoughts?

[jkmassel]

Most of the use of TARGET_OS_IPHONE is actually for iOS vs macOS support. Because this project has partial support for macOS, I'm thinking we'd want to evaluate the use of these constants on a case-by-case basis.

Paging @astralbodies – is macOS support something we ought to polish up and get working, or would it be preferable to clean it up until we need it for something? I'm happy to add that to my list, if needed :)

[astralbodies]

@jkmassel I believe we are using the Tracks library in Simplenote Mac. Right @jleandroperez?

[mindgraffiti]

Wow. I didn't know this was used in the map app too. Good call @jkmassel 👍

[jleandroperez]

Yup @jkmassel this is being used by Simplenote Mac (thanks for spotting that @astralbodies !!!).

[jkmassel]

@mindgraffiti – this should be mostly resolved in https://github.com/Automattic/Automattic-Tracks-iOS/commit/e6e6a197d8063360f93ecb8bf314d21c95d90085.

The code is a bit simpler now – each method exists no matter what, and we modify the internal implementation depending on the target. A bit more repeating ourselves, but I find it's far easier to work with.

One thing that may cause issues is the fact that unless you have a watch paired to the simulator, this block might add a note to the log about not having a paired device:

https://github.com/Automattic/Automattic-Tracks-iOS/blob/e6e6a197d8063360f93ecb8bf314d21c95d90085/Automattic-Tracks-iOS/TracksDeviceInformation.m#L115-L121

Given the fact that we don't do much watch development, I'm inclined to just return NO for simulators, but if we do, it'd then be incorrect down the road if we did start doing watch dev.

WDYT?

[mindgraffiti]

@jkmassel frankly, I don't know. @stevebaranski you have experience with watchOS dev right? What's your take on ☝️?

It's not really a decision I should be making because I wouldn't be the one to build a watch app (if one was created in the future).

[ghost]

@mindgraffiti & @jkmassel thanks for reaching out.

Looking at the underlying method, it looks like it's just checking value in NSUserDefaults. I am not concerned about this behavior on-device versus a Simulator.

If you encounter spurious logging related to Apple Watch, I'd guess that it's occurring in the initializer. It should be easy enough to test, but I think we should be good with the reliance on isSupported() there.

There are a few other things that might be worth mentioning.

1. The method named isAppleWatchConnected might be interpreted as "is a watch currently connected?" The method hasBeenPreviouslyPaired seems to answer the question: "has a watch ever been connected?". This is a small mismatch, but noteworthy.
2. In researching the issue, I noticed that we appear to have two pairs of repeated lines here. That might be worth tidying up.