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

Propagate interface fields #120

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

captbaritone
Copy link
Owner

@captbaritone captbaritone commented Mar 4, 2024

This PR changes how interface fields are handled. Previously, each interface implementor was responsible for adding /** @gqlField */ to each field. Even those which were already annotated on one of the interfaces it implements. This was tedious.

The one exception was fields defined with functions on interfaces. Here we would try to propagate the implementation to all implementors. However, there was a bug where we did not propagate the field to transitive implementors.

This PR addresses both issues by automatically propagating all fields to all implementors (including transitive implementors). If an implementor provides its own implementation of a field, the more specific implementation will be used.

Copy link

netlify bot commented Mar 4, 2024

Deploy Preview for grats ready!

Name Link
🔨 Latest commit 5b25f56
🔍 Latest deploy log https://app.netlify.com/sites/grats/deploys/65e54597459a38000830fa6c
😎 Deploy Preview https://deploy-preview-120--grats.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

}
-- TypeScript --
import { iThingGreeting as doohickeyIThingGreetingResolver } from "./concreteTypeInheritsFunctionFieldFromClosestTransitiveInterfacets";
import { greeting as widgetGreetingResolver } from "./concreteTypeInheritsFunctionFieldFromClosestTransitiveInterfacets";
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is incorrect. Widget should still be using iThingGreeting.

name: "greeting",
type: GraphQLString,
resolve(source) {
return doohickeyGreetingResolver(source);
Copy link
Owner Author

Choose a reason for hiding this comment

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

There's no "correct" answer for what this resolver should do. IThing and Entity each define a greeting field and their implementation's differ. Doohicky implements both equally so which should it choose?

Note, I think some languages with multiple inheritance leave it to ordering. The ordering of the inheritance becomes meaningful and the first one wins.

We likely need this to become a compile time error and force the user to define an explicit resolver for the type.

Thinking aloud: Another choice would be to start supporting the ability to define interfaces using (abstract?) classes and then only allow those interfaces to have concrete implementations. That would allow us to piggy-back on TypeScript only allowing single inheritance to ensure we always have a single unambiguous implementation for each field. However, it does not play well with functional style, where you want to be able to define your types as object literals or perhaps interfaces themselves.

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.

None yet

1 participant