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

[v1.0.0 alpha] PascalCase GraphQL operation names generate code that doesn't compile #2167

Closed
klanchman opened this issue Feb 18, 2022 · 7 comments

Comments

@klanchman
Copy link
Contributor

Bug report

When generating code from a GraphQL schema that has PascalCase query / mutation names, the generated code does not compile because of "ambiguous use of..." errors.

Versions

Please fill in the versions you're currently using:

  • apollo-ios SDK version: v1.0.0-alpha.1
  • Xcode version: Version 13.2.1 (13C100)
  • Swift version: 5.5.3
  • Package manager: SPM

Steps to reproduce

  1. Use a GraphQL server that has PascalCase operation names. In my case, https://graphql.anilist.co/ (GraphiQL here: https://anilist.co/graphiql)
    • For this example, I'll use the Staff query
  2. Write some GraphQL 👨‍💻
    query GetStaffDetail($id: Int!) {
        Staff(id: $id) {
            name {
                first
                last
                native
            }
        }
    }
  3. Generate code
  4. Build

The generated code looks like this:

public struct Data: AniListAPI.SelectionSet {
    // ...snip
    public static var selections: [Selection] { [
      .field("Staff", Staff?.self), // Compiler error: ⛔️ Ambiguous use of 'Staff'
    ] }

    public var Staff: Staff? { data["Staff"] }

    /// Staff
    public struct Staff: AniListAPI.SelectionSet {
        // ...snip
    }
}

This code doesn't compile because of the naming collision between public var Staff and public struct Staff.

The same query resulted in code that compiled successfully using apollo-ios v0.x.

Further details

I'm able to work around this by using aliases on all operations. If I tweak the example above:

query GetStaffDetail($id: Int!) {
    staff: Staff(id: $id) {
        name {
            first
            last
            native
        }
    }
}

This generates code that compiles:

public struct Data: AniListAPI.SelectionSet {
    // ...snip
    public static var selections: [Selection] { [
      .field("Staff", alias: "staff", Staff?.self),
    ] }
    
    public var staff: Staff? { data["staff"] }
    
    /// Staff
    public struct Staff: AniListAPI.SelectionSet {
        // ...snip
    }
}

This is less than ideal, since I'm likely to forget to alias new queries I write 😅

@AnthonyMDev
Copy link
Contributor

Thanks so much for pointing this out! We'll get this fixed for an upcoming Alpha version!
We'd love any other feedback you have on the alpha!

@AnthonyMDev AnthonyMDev added this to the Release 1.0 (Alpha) milestone Feb 21, 2022
@AnthonyMDev AnthonyMDev added this to Planned in Swift Codegen via automation Feb 21, 2022
@AnthonyMDev
Copy link
Contributor

@klanchman I'd like your opinion on how we should handle this.

It's a very easy fix to just make all the generated field names lowercased. I like this because it enforces Swift naming conventions, and it really simplifies this problem.

It basically would do what you suggest in your workaround without you needed to alias the fields.

So it would generate this:

public struct Data: AniListAPI.SelectionSet {
    // ...snip
    public static var selections: [Selection] { [
      .field("Staff", Staff?.self),
    ] }
    
    public var staff: Staff? { data["Staff"] }
    
    /// Staff
    public struct Staff: AniListAPI.SelectionSet {
        // ...snip
    }
}

Do you feel like it is valuable or necessary that we maintain the PascalCase 1:1 as is on the schema? That would require us to namespace variables, which would cause more complexity in the code generation logic, hurting performance, and generating uglier type names.

This would mean that ALL nested entity fields that are PascalCased would have to be name spaced also. In your example this isn't too bad, but imagine if Native were also a PascalCased field. You would get:

public struct Data: AniListAPI.SelectionSet {
    // ...snip
    public static var selections: [Selection] { [
      .field("Staff", Data.Staff?.self),
    ] }
    
    public var Staff: Data.Staff? { data["Staff"] }
    
    /// Staff
    public struct Staff: AniListAPI.SelectionSet {
        public static var selections: [Selection] { [
            .field("Native", Data.Staff.Native?.self),
        ] }

        public var native: Data.Staff.Native { data ["Native"] }
       
        /// Native
        public struct Native: AniListAPI.SelectionSet {
             // ...snip
        }
    }
}

I'd much prefer just enforcing that we make field names camelCased, but would like feedback on if this is acceptable to consumers.

@klanchman
Copy link
Contributor Author

@AnthonyMDev I would be happy with the first option, the automatic conversion to camelCase 👍
That's also what the v0.x codegen resulted in, and like you said is more Swift-y. I don't have any reason to keep the PascalCase names.


I'll be sure to keep filing feedback about the alpha! It's looking good so far migrating from v0.x using Swift scripting. I can't do much yet without operation variables but I saw those are on the way. Honestly the biggest hurdle I had so far was figuring out exactly how to set up codegen. I was getting some strange results due to file paths, but the example project from #2152 (comment) helped 😄

@AnthonyMDev
Copy link
Contributor

Okay great! In that case this is going to be easy and I'll have it in the Alpha 2!


'll be sure to keep filing feedback about the alpha! It's looking good so far migrating from v0.x using Swift scripting. I can't do much yet without operation variables but I saw those are on the way. Honestly the biggest hurdle I had so far was figuring out exactly how to set up codegen. I was getting some strange results due to file paths, but the example project from #2152 (comment) helped 😄

Yeah, I'm, getting operation variables done today! Once they are up, you can use the 1.0-alpha-incubating branch directly if you'd like to test them. The goal is to get include/skip directives finished as well before releasing Alpha 2. And I'll include the fix for this issue as well.

And, I agree we need much better documentation for onboarding. We are hoping to package the codegen and distribute it as a CLI, so the Swift Scripting won't be required. We really like using it personally, but are finding it's a barrier to entry for a lot of others.

Thanks for being an early user!

@AnthonyMDev
Copy link
Contributor

This has been merged into the 1.0-alpha-incubating branch. You can test it out there now! It will be included in the next alpha release (Alpha 2).

Swift Codegen automation moved this from Planned to Done Feb 22, 2022
@AnthonyMDev
Copy link
Contributor

I went ahead and released Alpha 2 today, so you can just rely on the tagged 1.0.0-alpha.2 version now!

@klanchman
Copy link
Contributor Author

Thanks! I’ll try out the new alpha soon, hopefully in the next few days 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Swift Codegen
  
Done
Development

No branches or pull requests

2 participants