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

Introduce TypeReference that allows linking types together without having to pass a reference to the actual type object or use callbacks #1149

Open
ruudk opened this issue May 23, 2022 · 12 comments
Milestone

Comments

@ruudk
Copy link
Contributor

ruudk commented May 23, 2022

When specifying a field type, I have to either provide the actual object instance, or use a callback that delegates the call to a registry to fetch the single instance of that type.

$filters = new InputObjectType([
    'name' => 'StoryFiltersInput',
    'fields' => [
        'tags' => [
            'type' => fn() => Type::nonNull( Type::listOf( $registry->get(SomeOtherType::class) ) ),
        ]
    ]
]);

I wonder why it works like that. Maybe this is for legacy reasons. But I would find it way easier if I could just link types by a FQCN or GraphQL type name.

So what if we create something like this:

final class TypeReference extends GraphQLType
{
    public function __construct(public readonly string $name)
    {
    }

    public function toString() : string
    {
        return $this->name;
    }
}
$filters = new InputObjectType([
    'name' => 'StoryFiltersInput',
    'fields' => [
        'tags' => [
            'type' => Type::nonNull( Type::listOf( new TypeReference(SomeOtherType::class) ) ),
        ]
    ]
]);

Now the type can be added to the schema. As soon as the type is needed, it needs to resolve the TypeReference to the actual type.
But that only happens when it is needed.

From an API perspective, it feels so much nicer to define things like this than to go with the callbacks + registry approach.

@spawnia
Copy link
Collaborator

spawnia commented May 24, 2022

How about TypeReference over DeferredType?

As soon as the type is needed, it needs to resolve the DeferredType to the actual type.

That would require including a type loader with the schema, not every schema has one. I guess we could throw when encountering a type reference but the schema has no type loader.

@ruudk
Copy link
Contributor Author

ruudk commented May 24, 2022

How about TypeReference over DeferredType?

I like it!

That would require including a type loader with the schema, not every schema has one. I guess we could throw when encountering a type reference but the schema has no type loader.

Sounds good

@ruudk ruudk changed the title Introduce DeferredType that allows linking types together without having to pass a reference to the actual type object or use callbacks Introduce TypeReference that allows linking types together without having to pass a reference to the actual type object or use callbacks May 24, 2022
@ruudk
Copy link
Contributor Author

ruudk commented May 24, 2022

Updated the PR to reflect TypeReference.

@spawnia
Copy link
Collaborator

spawnia commented May 24, 2022

Wouldn't it be nicer to reference types just by their name?

$filters = new InputObjectType([
    'name' => 'StoryFiltersInput',
    'fields' => [
        'tags' => [
            'type' => Type::nonNull(Type::listOf('SomeOtherType')),
        ]
    ]
]);

@ruudk
Copy link
Contributor Author

ruudk commented May 24, 2022

Nice, but why not then just use the string format that is already in the GraphQL schema / spec?

[SomeType]!

@spawnia
Copy link
Collaborator

spawnia commented May 25, 2022

I could see extending the functionality this way, but it is not necessary as a first step.

After looking into it a bit further, I can now see the fundamental issue with this change: a type with just a type reference is no longer able to fully function on its own. Take the StoryFiltersInput as an example: when it is not part of a schema, it is not possible to ask the type of its fields.

@ruudk
Copy link
Contributor Author

ruudk commented May 25, 2022

That's true, but why should it be fully possible to work on its own? Isn't that design decision (made long time) one of the reasons why we now have all these callbacks to make things lazy again?

I hope that for a future version of this wonderful library, a rewrite will undo all of this, and let types be simple and stupid with the Schema connecting everything together.

@spawnia
Copy link
Collaborator

spawnia commented May 25, 2022

One problem could be that types can be part of multiple schemas. While I have been following the one-schema approach, I know from Lighthouse that there is definite interest in having multiple schemas. Thus, the schema can not really be a part of types themselves.

A lot of APIs assume that types work by themselves. Consider ImplementingType::getInterfaces() and its many uses - if we introduced type references, we would somehow have to weave in the schema whenever calling it.

@ruudk
Copy link
Contributor Author

ruudk commented May 25, 2022

One problem could be that types can be part of multiple schemas.

Definitely, and I'm currently in the process of introducing another schema next to our large 250 type schema.

It's up to the developer to make sure that the Schema has a loader that is able to find those types.

Currently, this is done by directly linking types together and thus giving this certainty. But in bigger schema's (with lazy callable) this is often postponed via a locator/registry and then can still blow up when the type is not available.

So that would be the same as with the new approach right?

@spawnia
Copy link
Collaborator

spawnia commented May 25, 2022

I don't mind having the schema take the responsibility of type loading. Perhaps, it can even be beneficial to have context dependent types - for example, a referenced type could be different depending on which schema the type is in.

Again, the main issue I see is making all the current APIs work with it. Basically all methods on types, fields and argument objects that return a Type will no longer work outside the context of a schema. I would like to avoid an API that can be misused or that works only sometimes, so we would have to move the methods around: ImplementingType::getInterfaces() would become Schema::getInterfaces(ImplementingType $type). Same with probably a dozen other methods.

@ruudk
Copy link
Contributor Author

ruudk commented May 25, 2022

Again, the main issue I see is making all the current APIs work with it. Basically all methods on types, fields and argument objects that return a Type will no longer work outside the context of a schema. I would like to avoid an API that can be misused or that works only sometimes, so we would have to move the methods around: ImplementingType::getInterfaces() would become Schema::getInterfaces(ImplementingType $type). Same with probably a dozen other methods.

Yes it will be a lot of work. If we are going to move those responsibilities it would mean a breaking change, is that a problem?

Because if it's ok to break it then we can start to work on it.

@spawnia
Copy link
Collaborator

spawnia commented May 25, 2022

I am not planning to take on such a large scale change for the upcoming v15. It has plenty of breaking changes already and is nearing completion. Maybe after that, but it will be a while. Feel free to do a proof-of-concept and play with some ideas in the meantime. I think #1151 is not the way to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants