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 field inits to FragmentTemplate instead of just DataDict inits #2551

Closed
AmadeusK525 opened this issue Oct 6, 2022 · 13 comments
Closed
Assignees
Labels
codegen Issues related to or arising from code generation
Milestone

Comments

@AmadeusK525
Copy link
Contributor

Feature request

Generated Fragment objects should also have init methods with the necessary strictly-typed fields from the struct instead of just having the DataDict init, just like InputObjects

Motivation

The Fragment models generally contain the necessary data for the Views and ViewModels that will consume them, so I use them directly instead of creating mirror models and converting the dto into them (since that would just bloat the code).

So we avoid this:

class ViewModel {
    let model: MyModel

    public init(model: MyModel) {
        self.model = model
    }
}

struct MyModel {
    let id: String
    let someField: Int

    public init(from dto: MyFragment) {
        self.id = dto.id
        self.someField = dto.someField
    }
}

And do something like this instead:

class ViewModel {
    let dto: MyFragment

    public init(dto: MyFragment) {
        self.dto = dto
    }
}

When implementing previews for these Views, we need some easy Mock data and we used to be able to initialize these fragments with a normal init which just took the fields as arguments. With the new generation engine, this feature has been removed and has been made available only on InputObjects, so the only way to initialize these fragments is through a DataDict, and you can see how this is cumbersome (as stated in the test docs). The official Mock architecture is also available but it's a little overkill when all we need to do is:

let viewModel = ViewModel(dto: .init(id: "0", someField: 43))

Besides, using Mocks means that they'd need to be embedded into the main target of the application, which isn't desirable since we have a separate target for testing.

Proposed solution

Investigating the source code, InputObjectTemplate has its own functions that are used to define the convenient inits, so those would need to be externalized somehow so that they'd be able to be reused in more than one place. FragmentTemplate defines the inits by calling the SelectionSetTemplate's BodyTemplate function, the inits are defined in the SelectionSetTemplate.DataFieldAndInitializerTemplate function.

I'm not sure if it's desirable to implement this convenient init in the DataFieldAndInitializerTemplate function since I don't know to what extent that would affect other templates, so a solution to that may be needed

@calvincestari
Copy link
Member

Hi @AmadeusK525, thanks for reaching out about this.

With the new generation engine, this feature has been removed and has been made available only on InputObjects, so the only way to initialize these fragments is through a DataDict, and you can see how this is cumbersome (as stated in the test docs).

You're correct that this behaviour has changed; the generated response objects are now immutable. In the codegen proposal we stated our goal for doing this was to make the generated code far more concise and specific to purpose, which is to provide type-safe responses. Being able to mutate the fields on response objects cannot be used to affect anything server-side and in previous versions was a side-effect of the way cache mutations were designed. Cache mutations have been redesigned as well as test mocks, as you've noticed. So the functionality is still the same albeit refactored with more separation of concerns.

InputObjects have those initializers because their purpose is to accept data from the user and they can only be used in mutations. Which do go on to have an effect server-side.

so the only way to initialize these fragments is through a DataDict, and you can see how this is cumbersome (as stated in the test docs).

This is by-design, as stated above. Those objects are not meant to be created outside of receiving a GraphQL response.

Since it's neither a response, nor a test mock you're wanting to create it looks like we've encountered something outside our original considerations/requirements. If you have another proposal we're open to consider it but I do not believe we'd be willing to change the immutable nature of the response objects.

@AmadeusK525
Copy link
Contributor Author

Hey @calvincestari, thanks for the answer!

I must've misunderstood something, how would this make the object mutable? Making them immutable was one of the things I appreciated the most about the Release, so I definitely agree that it should stay that way.

Creating fragment objects doesn't mean that they'll be inserted in a Query's Data or anything like that, they'll be standalone data objects that aren't part of a direct GraphQL request (basically mocks).

struct PageInfoDetails: API.SelectionSet, Fragment {
    public static var fragmentDefinition: StaticString { """
        fragment PageInfoDetails on PageInfo {
            __typename
            endCursor
            hasNextPage
            hasPreviousPage
            startCursor
        }
        """ }

    public let __data: DataDict
    public init(data: DataDict) { __data = data }
    public init(
        endCursor: String?,
        hasNextPage: Bool?,
        hasPreviousPage: Bool?,
        startCursor: String?,
        variables: GraphQLOperaion.Variables? = nil
    ) {
        __data = DataDict([
            "endCursor": endCuror,
            "hasNextPage": hasNextPage,
            "hasPreviousPage": hasPreviousPage,
            "startCursor": startCursor,
        ], variables: variables)
    }

    public static var __parentType: ParentType { API.Objects.PageInfo }
    public static var __selections: [Selection] { [
        .field("endCursor", String?.self),
        .field("hasNextPage", Bool?.self),
        .field("hasPreviousPage", Bool?.self),
        .field("startCursor", String?.self),
    ] }

    public var endCursor: String? { __data["endCursor"] }
    public var hasNextPage: Bool? { __data["hasNextPage"] }
    public var hasPreviousPage: Bool? { __data["hasPreviousPage"] }
    public var startCursor: String? { __data["startCursor"] }
}

How would adding the init above change the mutability of the object?

@calvincestari
Copy link
Member

Yes, the initializer you have above doesn't change their immutability - my apologies you are correct.

Those objects aren't meant to be instantiated that way which is why we favour the generated code being concise. I still don't see a need for it in the generated code, you are always able to create an extension with that initializer yourself.

@calvincestari
Copy link
Member

After considering this issue we've decided not to implement this functionality - thank you.

@frehulfd
Copy link

FWIW our app has a very large schema, which makes adding bespoke initializers to each type infeasible. We utilize the init in V0 to create fixture data that are used for debug screens as well as other harnesses outside of mocking/unit-testing. We also had auto-generated (via Sourcery) fixtures that allowed developers to create fixture data without necessarily having to fill out every field.

Without a proper generated init we're going to have to investigate hacking something together with Sourcery or something similar.

@puls
Copy link
Contributor

puls commented Nov 10, 2022

I saw mention of hacking something together with Sourcery, so I did just that: https://gist.github.com/puls/af7be926b2270f45f8e20e389d6d65ff

My use case is automatically populating SwiftUI previews for views that are generally populated by GraphQL queries. I guarantee this Gist isn't a complete solution and only barely works for me but I hope it's useful for others who may stumble across this comment.

@yonaskolb
Copy link
Contributor

Just chiming in here to say the removal of type safe inits on Fragments are a big loss for us too. We use these to populate SwiftUI previews. As has been mentioned having field based inits is unrelated to immutability. This change makes Fragments terribly inflexible as types. I thought the whole point of Fragments was to be able to share them around and use them outside the context of a specific operation. This change basically forces us to create seperate models, leading to less re-use, code bloat and maintenance.
I suspect the push back on adding this back is because of climbing code size? If so, the client side requirement to implement these anyway negates that. I guess maybe a codegen option is called for.

@AnthonyMDev
Copy link
Contributor

Thanks for all the feedback on this one everyone. We agree that there needs to be an easier way to do this. We just aren't 100% sure what that looks like yet.

The primary problem here is that the generated initializers for fragments are pretty messy when you start getting into nested type conditions. Simple fragments aren't too much of an issue, but any solution we provide needs to handle any spec-compliant GraphQL fragment. We'll keep thinking about this in the new year and see what we can come up with.

@Nomad-Go
Copy link

I'm stuck on version 0.53.0 because of this. Not upgrading until I see support for Fragment initialization with individual properties.

@calvincestari
Copy link
Member

@Nomad-Go we're always happy to review PRs with suggestions to improve the library.

@AnthonyMDev
Copy link
Contributor

Just an update that we have finally come up with a solution for this that we are relatively happy with and I am in the process of implementing it. This will be part of the 1.1 release. I'll update this issue once it's completed so interested users can check out the 1.1 branch and test it out.

@bignimbus bignimbus added the codegen Issues related to or arising from code generation label Feb 10, 2023
@AnthonyMDev
Copy link
Contributor

The solution for this is up in PR #2869! Is it in the release/1.1 branch. We have a few things left that don't work 100% just yet. I'll be working on getting this stable for a release in the coming days.

In the mean time, we would really appreciate some early feedback! I'd like to make sure this does resolve the issues people are having and suits their needs.

If you are able to pull the release/1.1 branch, update your codegen config to include initializers and re-run code generation, please let me know how it works for you!

@AnthonyMDev
Copy link
Contributor

The 1.1 Beta is now live! Please check it out and let us know what you think! If you experience any bugs or this does not suit all of your needed use cases, let us know ASAP! We are aiming for official release of 1.1 next Friday (March 31st, 2023)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codegen Issues related to or arising from code generation
Projects
None yet
Development

No branches or pull requests

8 participants