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

feature: Emit Identifiable conformance on SelectionSets #548

Closed
wants to merge 1 commit into from

Conversation

x-sheep
Copy link

@x-sheep x-sheep commented Nov 26, 2024

This PR changes the code generator to emit Identifiable conformance on a SelectionSet where possible.

See also apollographql/apollo-ios#710 (comment)

@apollo-cla
Copy link

@x-sheep: 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/

Copy link

netlify bot commented Nov 26, 2024

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

Visit the deploys page to approve it

Name Link
🔨 Latest commit b7ef378

Copy link

netlify bot commented Nov 26, 2024

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

Visit the deploys page to approve it

Name Link
🔨 Latest commit b7ef378

@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Nov 26, 2024

✅ Docs Preview Ready

No new or changed pages found.

@calvincestari
Copy link
Member

Thanks for the PR @x-sheep. I appreciate your effort here but I don't think we can accept it into the library.

The main reason for this is the assumption this change makes that id is unique. While that may be true for your data it might not be true for everyone else that uses an id field. Take for instance a response object that is an interface named Animal and it returns Dog and Cat objects that conform to Animal. There is no guarantee that id is unique across both Dog and Cat instances while this change would impose the requirement that they must be unique.

Our team has discussed this before and our preference is to use a custom client directive that is declaratively applied to a schema type. This eliminates assumptions and it's then the responsibility of the developer to ensure the specified field adheres to being unique. If you want to make another attempt using that approach we can certainly provide design and review guidance.

@x-sheep
Copy link
Author

x-sheep commented Nov 26, 2024

@calvincestari According to Apple's documentation, the Identifiable protocol leaves the duration and scope of the identity unspecified. It is entirely valid to reuse IDs between different types of resources.

@calvincestari
Copy link
Member

I think a declarative mechanism is still a better choice for the library; it's explicit and can be applied to a wider set of use cases, such as objects that have identity but not through a field named id.

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.

4 participants