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

Don't add a raw type to Enums with no generated cases #519

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dwroth
Copy link

@dwroth dwroth commented Oct 25, 2024

When an enum has no cases, or when all of the existing cases have been deprecated, the generated code will not be compilable since Xcode/swift cannot automatically synthesize conformance to RawRepresentable.

This fix simply omits the String raw type when the enum has no cases, which allows the build to proceed.

Copy link

netlify bot commented Oct 25, 2024

👷 Deploy request for eclectic-pie-88a2ba pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 54a5366

Copy link

netlify bot commented Oct 25, 2024

👷 Deploy request for apollo-ios-docc pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 54a5366

@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Oct 25, 2024

✅ Docs Preview Ready

No new or changed pages found.

@dwroth dwroth changed the title Don't add an associated type to Enums with no generated cases Don't add a raw type to Enums with no generated cases Oct 25, 2024
@tt-pkaminski
Copy link

Hey @AnthonyMDev, any thoughts on this PR?

This addresses a bug we occasionally run into at our organization where a backend developer deprecates an enum type in the schema, and graphql codegen temporarily breaks by trying to create an enum type conforming to String without any cases.

E.g

# deprecated.graphql
enum ExperimentSignal {
    A
    B
}

# ExperimentSignal.graphql.swift

// Errors as we configure codegen not to generate deprecated enum cases
// and an enum without cases cannot conform to `String`
public extension GraphQL {
  enum ExperimentSignal: String, EnumType {
  }
}

@AnthonyMDev
Copy link
Contributor

I'm actually surprised that this builds for you. The EnumType protocol requires that the type be RawRepresentable where RawValue == String. I guess with no cases in the enum, this requirement is still fulfilled?

If it's building properly, I don't have an issue with the fix though! We had some CI tests fail due to Xcode 16.1 moving out of beta, which should be fixed now. Let us make sure this passes CI first!

@AnthonyMDev
Copy link
Contributor

@calvincestari, I thought this would be working when I re-run it after #522 was merged. It's still failing. Do we need to rebase this on main or is something else wrong?

@calvincestari
Copy link
Member

@calvincestari, I thought this would be working when I re-run it after #522 was merged. It's still failing. Do we need to rebase this on main or is something else wrong?

It will need to be rebased yes. I can take care of that..

@tt-pkaminski
Copy link

That's a good callout. I tried this in a playground and it does throw an error.

protocol EnumType: RawRepresentable where RawValue == String {}
enum TestEnum: EnumType {}

I'm not too familiar with the repository set up, but do you know if generated code will build here @dwroth ?

@dwroth
Copy link
Author

dwroth commented Oct 30, 2024

That's a good point. Let me test it against our repo to be sure.

@calvincestari
Copy link
Member

@AnthonyMDev - this is now successfully building against our tests but you make a good point about whether the change is actually valid Swift code. Which seems to be confirmed as not by @tt-pkaminski's comment.

@dwroth
Copy link
Author

dwroth commented Oct 30, 2024

You're right, it's not valid swift code. 🤦‍♂️

I'm not sure that there's actually a way around this. Even when all of the enum cases are deprecated, there may still be references to the enum type in the project, so we probably shouldn't just remove the type entirely. But leaving it in creates a "does not conform to RawRepresentable" error.

@calvincestari
Copy link
Member

I'm going to put this PR into draft so it doesn't get merged accidentally. When we figure out if there is a solution we can move forward with it again.

@calvincestari calvincestari marked this pull request as draft November 1, 2024 19:56
@dwroth
Copy link
Author

dwroth commented Nov 4, 2024

Thanks, @calvincestari.

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.

5 participants