-
-
Notifications
You must be signed in to change notification settings - Fork 565
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
Comments
How about
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. |
I like it!
Sounds good |
DeferredType
that allows linking types together without having to pass a reference to the actual type object or use callbacksTypeReference
that allows linking types together without having to pass a reference to the actual type object or use callbacks
Updated the PR to reflect |
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')),
]
]
]); |
Nice, but why not then just use the string format that is already in the GraphQL schema / spec?
|
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 |
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. |
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 |
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? |
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 |
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. |
I am not planning to take on such a large scale change for the upcoming |
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.
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:
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.
The text was updated successfully, but these errors were encountered: