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

SwiftUI previews fix #1066

Merged

Conversation

TizianoCoroneo
Copy link
Contributor

Hello @designatednerd 👋
Thank you for your amazing work on this library! I had the same issue reported here and decided to try to help out.

This PR fixes an issue that started with the inclusion of ApolloCodegenLib in 0.23.0, where all SwiftUI previews for files in targets that also import Apollo stopped working with the following error:

use of unresolved identifier 'Process'

Compile /Users/peck/Library/Developer/Xcode/DerivedData/practice-ffshylstkbdpuwdygwokyrmihofw/SourcePackages/checkouts/apollo-ios/Sources/ApolloCodegenLib/Basher.swift:
/Users/peck/Library/Developer/Xcode/DerivedData/practice-ffshylstkbdpuwdygwokyrmihofw/SourcePackages/checkouts/apollo-ios/Sources/ApolloCodegenLib/Basher.swift:27:16: error: use of unresolved identifier 'Process'
    let task = Process()

In my case, I have Apollo as a dependency using SPM. I don't know if this issue is reproducible with cocoapods or other dependency managers.

This happens because ApolloCodegenLib is meant to be built only on macOS, and this check is only enforced with a runtime @available annotation instead of a compile-time #if os(macOS).
I looked for a way to specify a specific platform per-target directly in the SPM manifest, but according to this PR in the SPM repo there is no way for now (and he recommends to strip away the platform specific code using #if):

Platform-specific targets or products: Consider that a package supports multiple platforms but has a product that should be only built for Linux. There is no way to express this intent and the build system may end up trying to build such targets on all platforms. A simple workaround is to use #if to conditionalize the source code of the target. Another use case comes up when you want to keep deployment target of a certain target lower than other targets in a package. A workaround is factoring out the target into its own package. A proper solution to this problem will be explored in a separate proposal, which should provide target-level settings.

This PR applies the suggestion specified above. I'm sorry it is not below 20 lines and I'm not really following the contributor guide, but this seems an "innocuous" bug fix. 😄

I don't know how to test this behavior with SwiftUI previews to be honest.

@apollo-cla
Copy link

@TizianoCoroneo: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@designatednerd
Copy link
Contributor

Hey thank you for doing this! I think you’re being slightly too aggressive adding it to every file - it should really only be the files I marked with @available which should need this. I can’t believe I forgot that was runtime only. 🤦‍♀️

It does seem to be a combo SPM/SwiftUI bug that this lib gets compiled for SwiftUI even when it’s not a dependency of the target being compiled though. Will file a radar on that one.

…prevent compilation on other platforms. This previously caused an issue that prevented SwiftUI previews on iOS targets from working properly, if that target included Apollo as a dependency.
@TizianoCoroneo
Copy link
Contributor Author

I removed the extra #if(macOS from the source files that didn't contain @available before.
Tested with my SwiftUI previews and it works 👍

@designatednerd
Copy link
Contributor

Awesome, thanks so much for this!

@designatednerd designatednerd merged commit 5751ad3 into apollographql:master Mar 5, 2020
@designatednerd designatednerd added this to the Next Release milestone Mar 5, 2020
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.

3 participants