Orphaned Types

[twavv]

I've been opening up several issues on this project lately (because I think it's pretty darn cool!). I'd like to devote this one to talking about orphaned types as it doesn't seem there's a single place where that discussion has been taking place.

In the 0.18 beta, there is a concept of orphan types, that is, types that exist (are declared with decorators) but aren't explicitly referenced in the schema. The reason for this is the desire to support non-global schemas (e.g., we want to support more than two schemas in the same node process).

The main issue with the current state of things (as of the 0.18-beta) is that it doesn't work super well for interfaces (at the very least, the behavior is unintuitive). Suppose you have an interface Animal, a query getAnimal that returns an Animal, and several implementations such as Cat, Dog, etc.. Currently, if Cat and Dog aren't included elsewhere in the schema (e.g., in a query that explicitly returns a Dog instead of an Animal), then Cat and Dog are considered oprhaned types and are not added to the GraphQL schema by default. The end result of this is that there is no way to actually use the Animal type (since all implementations aren't actually defined in the schema).

Proposals

Explicit orphanedTypes argument (status quo)

This is the status quo.

• Pro: Avoids all potential issues with duplicate schemas
• Con: Has issues with interfaces (described above)
• Con: All orphaned types have to be supplied in buildSchema which makes it harder to be modular
• Con: This is very unintuitive behavior (especially if you're used to the existing type-graphql behavior)

Add an implementedBy field to @InterfaceType decorator

Proposed in #110 (comment).

• Pro: More modular than first option
• Con: Likely to have issues with circular dependencies

Auto-register interface implementations

This is my preference. Essentially, internally have some metadata associated with each interface type, and when the interface type is added to the schema, also register all known implementations at that time.

• Pro: Fully modular
• Pro: No circular dependencies
• Pro: More intuitive (at least with respect to current type-graphql usage)
• Con: It's possible to "forget" to include a class (if it's not explicitly imported). This is the case with 0.17 and also not that big of a deal since if it hasn't been imported, it can't be instantiated anyway (assuming you're using class instances).

Explicit Schema Registration

This is a huge departure from the way things are now, but I thought I'd mention it. In (for example) Python's Flask, you can register routes using decorators:

@app.get("/foo")
def get_foo():
    ...

We could do something similar where we have an explicit schema object and the decorators are instance methods.

@schema.ObjectType()
class ... { /* ... */ }

Alternatively, the schema could be an option to the decorator, and if not provided, use a global default.

@ObjectType({
  // If omitted, this defaults to the "global" schema
  schema: internalSchema
})
class ... { /* ... */ }

• Pro: Very explicit about what objects are associated with which schemas
• Con: Huge departure from the way things are today
• Con: Circular dependency issues (Flask has lots of these)

---

The third proposal (Auto-register interface implementations) is my favorite I'd be happy to take a stab at this. It's also a minor change from the way things are in the 0.18-beta codebase.

[MichalLytek]

Likely to have issues with circular dependencies

Not really - if it would be implemented with the function syntax, like implemetedBy: () => [Foo, Bar].

Auto-register interface implementations

It's again just a trade-off - instead of explicit import to provide orphanedTypes, we rely on the auto register like with the v0.17.x which means you need to have a dummy imports just to trigger decorators evaluation that puts the metadata in the storage.

if it hasn't been imported, it can't be instantiated anyway (assuming you're using class instances)

As we now can use interfaces and union without instances, it's a no go to require dummy import (as TS has now the import type syntax).

Explicit Schema Registration

It kinda reminds me the connection or database syntax from TypeORM:
https://typeorm.io/#/multiple-connections/using-multiple-databases-in-a-single-connection

But as the "auto import" was just a bug because of my laziness, not a feature, I am against the auto import, so this feature makes no sense if you explicitly provide types and resolvers to separate buildSchema calls.

I think the best way to handle that interface problem is the implementedBy with the fallback to auto schema registering if nothing is provided. Because only the interfaces suffer from the orphaned types problem, there's no sense to make some more generic solution.

But anyway, thanks @travigd for this really long showcase of all the possibilities, I appreciate that ❤️

[twavv]

Not really - if it would be implemented with the function syntax, like implemetedBy: () => [Foo, Bar].

I suppose this is true, but it still is a circular dependency (whether or not that's actually an issue beyond ideological purity is up for debate).

My argument against this option is this: The point of an interface is that it generally shouldn't be concerned with what it is implemented by (otherwise we might as well just use a union). The issue with that approach though is that we need to know (when we're building the schema) all of the concrete object types that actually implement the interface.

I think the best way to handle that interface problem is the implementedBy with the fallback to auto schema registering if nothing is provided. Because only the interfaces suffer from the orphaned types problem, there's no sense to make some more generic solution.

This seems reasonable to me. The thing I would like to see the most is just the intuitive behavior of "if I include an interface in the schema, then all objects that use that interface are also registered to that schema."

I think that the actual scenario where two different schemas have a disjoint set of objects that implement the same interface would be rare. The only exception that comes to mind is the Relay-style Node interface, where both a public and a private schema might implement Node.

All of that is to say that I agree, but I think the "default" should be reversed: auto-registering should be considered the default and implementedBy should be the "if you need to do something special" case. It only really matters for documentation and examples, but, those are my ten cents.

Edit: I suppose another alternative in the same vein would be to have just an autoregisterObjects (or similar) property for the @InterfaceType decorator. Setting this to false would act the same as it does in 0.18, which is to require the object types to be included in orphanedTypes.

But anyway, thanks @travigd for this really long showcase of all the possibilities, I appreciate that ❤️

I like a lot of things about this project! I've opened a number of issues in the last few days because I've started to run up against some of the things that I want to do but that typegraphql doesn't support (whether or not they're good things to do are up for debate 😉), but I'm very grateful for all the work that has been done on this project - it's making my life much easier! I hope that's coming through.

[MichalLytek]

The point of an interface is that it generally shouldn't be concerned with what it is implemented by

It's not true in GraphQL because you need to provide resolveType function which enforce having known about all the implementations 😄

(otherwise we might as well just use a union).

Union doesn't enforce common fields and doesn't allow to query for them without specifying the ... case.

we need to know (when we're building the schema) all of the concrete object types

Treat this as a submodule point to register orphanedTypes.
We could support both options - implementedBy and then orphanedTypes.

The only exception that comes to mind is the Relay-style Node interface, where both a public and a private schema might implement Node.

Don't you think it's a pretty common case? So we can't enforce implementedBy as this would break this kind of schemas. That's where orphanedTypes workaround could be used as I have no other API proposal for that case while still having schemas separation.

auto-registering should be considered the default and implementedBy should be the "if you need to do something special" case

I'm afraid auto-registering won't be possible with the vNext which has metadata storage design that prevents memory leaks that are a pain in the ass of current version.
So everything needs to be explicit - either by being referenced as a type of field, or by orphanedTypes, or by implementedBy.

[twavv]

It's not true in GraphQL because you need to provide resolveType function which enforce having known about all the implementations 😄

Yeah, that's true when we're building the schema. But typegraphql is an abstraction on top of graphql-js, so I'm not sure it necessarily needs to have the same requirement (since we can generate the resolveType function when we go to build the schema - which is what we currently do).

I guess what I'm trying to say here is: Ideally, interfaces shouldn't need to know about their implementation. Building a GraphQL schema does require us to know about their implementations. It would be nice™ to not expose that to devs using typegraphql if they can avoid it.

It's more so a matter of what kind of interface typegraphql wants to expose to devs using it (and very much subject to opinion!). It seems (to me) that since typegraphql is a tool used to abstract the building of a GraphQL schema, if it can abstract away that annoying aspect of requiring resolveType, it should (and it seems very much possible).

I'm afraid auto-registering won't be possible with the vNext which has metadata storage design that prevents memory leaks that are a pain in the ass of current version.

I'm not familiar with the storage design of vNext or the existing memory leak issues so I can't really speak to that. But I think that autoregistration would still be possible no matter what? If you're not writing decorator/metadata storage to a global store, I would still think it's possible since object types still need to explicitly use implements: [MyInterface] (and so you'd be able to access the storage associated with MyInterface and add it to a list of implementations for auto-registration).

[MichalLytek]

(and so you'd be able to access the storage associated with MyInterface and add it to a list of implementations for auto-registration).

I use a WeakMap where the key are classes constructors. So I would need to plug in somewhere inside the metadata building process to store the mapping between object type and interface type implements in another WeakMap 🤔
So it's doable, will figure out when it will be it's turn to implement 😄

[twavv]

Yay! Sounds like a plan to me. 😎

Feel free to loop me in if you'd like some help with some of those things.

[MichalLytek]

I suppose another alternative in the same vein would be to have just an autoregisterObjects (or similar) property for the @InterfaceType decorator. Setting this to false would act the same as it does in 0.18, which is to require the object types to be included in orphanedTypes.

@travigd
This has been implemented in #595 with autoRegisterImplementations: false setting - please check and review the PR 😉