-
-
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 StandardTypeRegistry #1426
Conversation
It's outside the scope of this PR, but in the future, we could also ship 2 interfaces that can be implemented on the /**
* A registry that lazily initializes types by their class name.
*/
interface LazyInitializedFullyQualifiedTypeRegistry
{
/**
* @template TType of Type&NamedType
*
* @param class-string<TType> $type
*
* @return (callable(): TType)|TType
*/
public function byClass(string $type);
} /**
* A registry that returns types by their name.
*/
interface NamedTypeRegistry
{
/** @return Type&NamedType */
public function byName(string $name);
} That would also allow us to deprecate/remove the typeLoader as this can be now implemented using the type registry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not get through all files just yet, this is just some immediate feedback I can give you right now.
ecfb671
to
cc96ad6
Compare
Currently, types are referenced and cached statically. This is problematic when using multiple schema's that have different standard types that share the same memory. For example, when running them in un-isolated unit tests or when there is a long running PHP process that serves GraphQL requests. To solve this problem, we introduce a `StandardTypeRegistry` interface with a `DefaultTypeRegistry` implementation. People are allowed to create their own registries by implementing the interface. Every Schema should be constructed with a `typeRegistry` that is an instance of `StandardTypeRegistry`. From there on, all types are queried from the registry. The registry will be responsible for caching the types to make sure subsequent calls to the same type will return the same instance. Internally, all static calls to the standard types (Type::string(), Type::int(), Type::float(), Type::boolean(), Type::id()) have been replaced with dynamic calls on the type registry. Also calls to the introspection objects and internal directives are using the type registry now. As most people probably have only one schema, we keep the static methods on the Type call. These now forward to `DefaultTypeRegistry::getInstance()`. This way, we don't break existing installations. The reason for creating a `StandardTypeRegistry` interface as opposed to just a non-final implementation is that it allows people to use composition instead of inheritance to extend the functionality of the registry. For example, in my project I'd like to have a registry that holds all types and that allows me to query any type by their name (instead of FQCN). I can then delegate the interface methods to the decorated `StandardTypeRegistry`. Resolves webonyx#1424
d746a7f
to
fb12480
Compare
We now store this separately.
I removed the Introspection from the StandardTypeRegistry interface. It's now stored separately on the Schema and passed along the schema. Still not convinced. I think the Introspection class is weird. 💡 I have an idea: Maybe, we can solve all these problems, by creating something completely different than what this PR does:
|
@@ -467,7 +466,7 @@ protected function shouldIncludeNode(SelectionNode $node): bool | |||
$variableValues = $this->exeContext->variableValues; | |||
|
|||
$skip = Values::getDirectiveValues( | |||
Directive::skipDirective(), | |||
$this->exeContext->schema->typeRegistry->skipDirective(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still feels off, why would the $typeRegistry
deal with directives?
@@ -99,6 +107,9 @@ public function __construct($config) | |||
$this->extensionASTNodes = $config->extensionASTNodes; | |||
|
|||
$this->config = $config; | |||
|
|||
$this->typeRegistry = $config->typeRegistry ?? DefaultStandardTypeRegistry::instance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, the names $typeRegistry
and DefaultStandardTypeRegistry
do not accurately reflect that the expected object also deals with directives. I guess it makes sense for those interfaces to travel together, given that the directives functionality requires the built-in types.
Overall, I think we can get rid of the term standard types, it is never used in the GraphQL specification. Instead, it refers to built-in scalars and built-in directives. Both types and directives are considered a TypeSystemDefinition, or perhaps short definition. So how about we recombine the interfaces and call it BuiltInDefinitionRegistry
, implement it as DefaultBuiltInDefinitionRegistry
and refer to it as builtInDefinitionRegistry
?
@spawnia what do you think about #1426 (comment) |
Sounds interesting, perhaps you can illustrate that approach in a second pull request and allow us to compare? |
@spawnia Sounds good. Will work on it! |
I could really use something like this, what's the status @ruudk? |
I'm commenting from an implementation perspective and not an internal one. Why isn't this a container? Am I just viewing it too shallow? This is where I store my types and were there a standard type registry I suppose I would implement against it: This is nearly bare-bones PSR-11 and is "standard type" and not introspective. I use many containers and allow overriding of their registry. That way my application can use a shared type manager if I create two drivers. |
Currently, types are referenced and cached statically. This is problematic when using multiple schema's that have different standard types that share the same memory. For example, when running them in un-isolated unit tests or when there is a long running PHP process that serves GraphQL requests.
To solve this problem, we introduce a
StandardTypeRegistry
interface with aDefaultTypeRegistry
implementation. People are allowed to create their own registries by implementing the interface. Every Schema should be constructed with atypeRegistry
that is an instance ofStandardTypeRegistry
. From there on, all types are queried from the registry. The registry will be responsible for caching the types to make sure subsequent calls to the same type will return the same instance.Internally, all static calls to the standard types (Type::string(), Type::int(), Type::float(), Type::boolean(), Type::id()) have been replaced with dynamic calls on the type registry. Also calls to the introspection objects and internal directives are using the type registry now.
As most people probably have only one schema, we keep the static methods on the Type call. These now forward to
DefaultTypeRegistry::getInstance()
. This way, we don't break existing installations.The reason for creating a
StandardTypeRegistry
interface as opposed to just a non-final implementation is that it allows people to use composition instead of inheritance to extend the functionality of the registry. For example, in my project I'd like to have a registry that holds all types and that allows me to query any type by their name (instead of FQCN). I can then delegate the interface methods to the decoratedStandardTypeRegistry
.Resolves #1424