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 #584

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

x-sheep
Copy link
Contributor

@x-sheep x-sheep commented Jan 23, 2025

This is a rework of #548, except this time it will only emit a conformance if the keyFields is set to exactly ["id"] (instead of assuming the presence of an id field is enough).

If a type has a field named id it is actually impossible to override the behavior of Identifiable, so I believe there is no situation in which an automatic conformance to Identifiable isn't desirable.

This PR will not generate a getter named id (as was proposed earlier), so there won't be an unexpected field named id in any case.

Copy link

netlify bot commented Jan 23, 2025

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

Visit the deploys page to approve it

Name Link
🔨 Latest commit 450417e

Copy link

netlify bot commented Jan 23, 2025

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

Visit the deploys page to approve it

Name Link
🔨 Latest commit 450417e

@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Jan 23, 2025

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: 2f9d869c04823e5db25d5510

@calvincestari
Copy link
Member

I think it would be beneficial to include a test that has id as the key field but it is not included in the selection set; test should not generate Identifiable conformance.

// given
schemaSDL = """
type Query {
  allAnimals: [Animal!]
}
type Animal @typePolicy(keyFields: "id") {
  id: ID!
  species: String!
}
"""

document = """
query TestOperation {
  allAnimals {
    species
  }
}
"""

Copy link
Member

@calvincestari calvincestari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One formatting suggestion.


/// Indicates if the parent type has a single keyField named `id`.
public var isIdentifiable: Bool {
guard direct?.fields ["id"] ?? merged.fields["id"] != nil else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
guard direct?.fields ["id"] ?? merged.fields["id"] != nil else {
guard direct?.fields["id"] ?? merged.fields["id"] != nil else {

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