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 request] Resolvers for interfaces #260

Closed
samdenty opened this issue Feb 23, 2019 · 10 comments · Fixed by #579
Closed

[Feature request] Resolvers for interfaces #260

samdenty opened this issue Feb 23, 2019 · 10 comments · Fixed by #579
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request
Milestone

Comments

@samdenty
Copy link

samdenty commented Feb 23, 2019

Is your feature request related to a problem? Please describe.
Currently resolvers can only be applied to ObjectTypes

Describe the solution you'd like
If an InterfaceType is specified in the params to a resolver, all @FieldResolvers on the interface will apply to all ObjectType implementations.

If a resolver for an ObjectType already has a FieldResolver for a select field, it should use the ObjectType's definition (instead of the InterfaceType)

@InterfaceType()
class Test {
  @Field() firstName: string
  @Field() lastName: string
  @Field() fullName: string
}

@ObjectType({ implements: Test })
class Test2 extends Test {}

@ObjectType({ implements: Test })
class Test3 extends Test {}

@Resolver(of => Test)
class TestResolver {
  @FieldResolver()
  fullName(@Root() test: Test) {
    return test.firstName + test.lastName
  }
}
@MichalLytek MichalLytek added Enhancement 🆕 New feature or request Community 👨‍👧 Something initiated by a community Discussion 💬 Brainstorm about the idea Need More Info 🤷‍♂️ Further information is requested labels Feb 23, 2019
@samdenty
Copy link
Author

Or alternatively a way to compose resolvers

@Resolver()
class TestResolver {
  @FieldResolver()
  fullName(@Root() test: Test) {
    return test.firstName + test.lastName
  }
}

@Resolver(of => Test2)
class Test2Resolver extends TestResolver {}
...

@MichalLytek
Copy link
Owner

I've always treated GraphQL interface like a normal TS interface, that is pure abstract and doesn't have an implementation. I thought that it's better to just use a base object type for fields with resolver and args and just extend them.

@ObjectType()
class Person {
  @Field()
  age: number;
}

@ObjectType()
class Student extends Person {
  @Field()
  universityName: string;
}

Also, mixing the extending class type is prohibited - your object type can't extend an interface type:
https://19majkel94.github.io/type-graphql/docs/interfaces-and-inheritance.html

Note that both the subclass and the parent class must be decorated with the same type of decorator, like @ObjectType() in the example Person -> Student above. Mixing decorator types across parent and child classes is prohibited and might result in schema building error - you can't e.g decorate the subclass with @ObjectType() and the parent with @inputType().

When graphql-js calls the field resolver of the InterfaceType? For common interface field of the interface return type of the query? Or it always delegate the field resolving to the proper ObjectType and just ignores the interface resolvers?

Inheriting resolvers looks like a non standard solution that has to be handled specially:
ardatan/graphql-tools#720

@samdenty
Copy link
Author

samdenty commented Feb 23, 2019

@19majkel94 How about allowing for composition of resolvers at the bare minimum? Doesn't go against the graphql-js spec, just a way of increasing code-reuse

@InterfaceType()
class Test {
  @Field() firstName: string
  @Field() lastName: string
  @Field() fullName: string
}

@ObjectType({ implements: Test })
class Test2 extends Test {}

@ObjectType({ implements: Test })
class Test3 extends Test {}

@Resolver()
class TestResolver {
  @FieldResolver()
  fullName(@Root() test: Test) {
    return test.firstName + test.lastName
  }
}

@Resolver(of => Test2)
class Test2Resolver extends TestResolver {}

@Resolver(of => Test3)
class Test3Resolver extends TestResolver {}

@MichalLytek
Copy link
Owner

MichalLytek commented Feb 23, 2019

Have you tried that?

How about allowing for composition of resolvers at the bare minimum?

It's perfectly legal to have a field resolver in base resolver class:

https://github.com/19majkel94/type-graphql/blob/526886c9cf0e2a951e0ed5fe45d8d00ed4fc3675/examples/resolvers-inheritance/resource/resource.resolver.ts#L54-L58

@samdenty
Copy link
Author

ah didn't know that was possible thanks

@MichalLytek
Copy link
Owner

As graphql-tools supports inheritResolversFromInterfaces, it might be a good idea to support this kind of feature too.

Of course after the #261 arrives.

@MichalLytek MichalLytek added Blocked 🚫 Resolving this issue is blocked by other issue or 3rd party stuffs and removed Discussion 💬 Brainstorm about the idea Need More Info 🤷‍♂️ Further information is requested labels Mar 3, 2019
@MichalLytek MichalLytek added this to the 2.0.0 release milestone Mar 3, 2019
@chrisdostert
Copy link

chrisdostert commented Feb 12, 2020

@MichalLytek Not having this breaks paginating any field which is an interface type (w/ multiple implementations) in relay. Relay only allows paginating a single connection within a fragment. If you can't define the connection on the interface type then you can't paginate it. Side note; in the spirit of getting this rolled with some priority, would donating some money help? I don't have time to roll it myself right now and it seems like you're not satisfied with the implementation @kl4n4 did.

@MichalLytek
Copy link
Owner

MichalLytek commented Feb 12, 2020

@chrisdostert If you need this urgently, you can just use his branch of the fork he made.

My priority is vNext when the internal architecture is made properly and I can add features like this easily and quickly 😉

@MichalLytek MichalLytek linked a pull request Mar 15, 2020 that will close this issue
@MichalLytek MichalLytek removed the Blocked 🚫 Resolving this issue is blocked by other issue or 3rd party stuffs label Mar 15, 2020
@MichalLytek
Copy link
Owner

MichalLytek commented Mar 23, 2020

Closed by #579, released in v0.18.0-beta.14 🎉

@samdenty @chrisdostert @justinmchase @hyperloris @dmks
Let me know guys how it works for you 😉
I would like to roll out the stable release soon but I can't do that without confirmation on this feature.

@justinmchase
Copy link

justinmchase commented Mar 23, 2020

@MichalLytek Ok cool, so you can define the resolver for the field either on the interface itself or on the resolver?

https://github.com/MichalLytek/type-graphql/pull/579/files#diff-b81ada8bdad46121b8aa68dd67d9238aR145

So yeah that seems like it would work perfect because I would need access to services and the resolver will be able to have service dependencies resolved I believe. So that all looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants