Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic watchOS support to better support multi platform apps #78

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

KaiOelfke
Copy link

@KaiOelfke KaiOelfke commented Oct 7, 2023

This PR is mainly to prevent compile time errors and doesn’t implement a good UX on watchOS. I have a multiplatform app. With Xcode 15 I can't build my app anymore as Roadmap fails to compile for watchOS. I think in most cases a Roadmap doesn't make sense on watchOS. But I'm not aware of a good way to restrict platforms for a Swift package.

https://forums.swift.org/t/restrict-platforms-for-packages/29456/7

Not mentioning watchOS in platforms just means the oldest watchOS version is supported implicitly.

I'm not 100% sure yet why Xcode even tries to compile Roadmap for watchOS in my project. I only have it as a dependency in my own Package.swift for a library target that is only added to an iOS target. I also added a TargetDependencyCondition .product(name: "Roadmap", package: "Roadmap", condition: .when(platforms: [.iOS])), And the actual view is surrounded with #if os(iOS) #endif But this iOS target also embeds my watchOS target.

This PR only fixes the errors. I didn't make any other changes so the watchOS UX is probably not very good. I don't plan to use Roadmap on my watchOS app anyway. But this is how the preview looks.

Bildschirmfoto 2023-10-07 um 12 41 16

Edit: I missed another dependency that was also depending on Roadmap. Adding condition: .when(platforms: [.iOS])) there as well fixed things. So now it's not building for watchOS anymore. Therefore I don't need these changes anymore. But maybe it's still useful for others to have this basic compatibility. Then you don't need to constrain everything.

This commit is mainly to prevent compile time errors and doesn’t implement a good UX on watchOS.
@hiddevdploeg
Copy link
Collaborator

hiddevdploeg commented Oct 11, 2023

Hi @KaiOelfke, I'm not sure I understand the problem. You can exclude Roadmap from the watchOS target, and it compiles fine?

I'm using Roadmap on 4 different apps; all have a watchOS target, and I've never faced a problem.

@KaiOelfke
Copy link
Author

KaiOelfke commented Oct 11, 2023

Hi @KaiOelfke, I'm not sure I understand the problem. You can exclude Roadmap from the watchOS target, and it compiles fine?

I'm using Roadmap on 4 different apps; all have a watchOS target, and I've never faced a problem.

It sounds like you added Roadmap directly to frameworks, libraries and embedded content in Xcode for the iOS target, but not watchOS. And yes this should work. But my setup is different.

I only have a MySwiftPackageFramework target in the watchOS app target. In this MySwiftPackageFramework I had two dependencies that depend on Roadmap. In one of them I excluded watchOS via:

// Before, not working
.product(name: "Roadmap", package: "Roadmap")
// Now, (compiles fine}
.product(name: "Roadmap", package: "Roadmap", condition: .when(platforms: [.iOS]))

In the other one I forgot this. Which caused the compiler issues. After making the PR I found out that I missed the exclude in the second dependency and wrote the edit in my PR comment above. So as I said above I don't need the changes of this PR anymore. And I decided to leave it up to the community, if people want a basic watchOS compatibility or not. With this PR people can integrate Roadmap easier without having to understand how to properly exclude Roadmap.

For example if someone only has

#if os(iOS)
import Roadmap
RoadmapView() 
#endif

This isn't enough. They either need to exclude it in the watchOS app target or in the Swift Package dependency depending on their setup.

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.

2 participants