-
Notifications
You must be signed in to change notification settings - Fork 9
feat: entity type customization, type loader support #33
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
Conversation
@@ -149,28 +181,12 @@ private function getQueryTypeServiceFieldConfig(): array | ||
/** @var array */ | ||
private function getQueryTypeEntitiesFieldConfig(?array $config): array | ||
{ | ||
if (!$this->hasEntityTypes()) { |
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 is removed because it is incompatible with the notion of an invocable set of union types. I don't believe it is needed since the _entities
query is always a required part of the subgraph definition
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.
Is there a reference you can link here?
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.
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.
There are various similar links throughout the readme, this is the one that specifies you have to include _entities
@@ -4,17 +4,17 @@ | ||
|
||
namespace Apollo\Federation; | ||
|
||
use Apollo\Federation\Types\AnyType; |
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.
Import ordering, this should be caught by linting
@@ -55,18 +55,30 @@ | ||
*/ | ||
class FederatedSchema extends Schema | ||
{ | ||
/** @var EntityObjectType[] */ | ||
/** @var EntityObjectType[]|callable: EntityObjectType[] */ |
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 haven't seen the callable: ReturnType
type declaration before- when was this introduced?
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 actually adapted it from here: https://github.com/webonyx/graphql-php/blob/master/src/Type/Definition/UnionType.php#L18
I'm not sure when it was introduced, but I think it's apt that we're mimic'ing the union type structure.
*/ | ||
private function supplementTypeLoader(array $config): array | ||
{ | ||
if (!array_key_exists('typeLoader', $config) || !is_callable($config['typeLoader'])) { |
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.
We're using the 'typeLoader'
string a lot- we should share this value in a way that we understand the type signature of the config and expose that externally as well.
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.
The typeLoader string is a constant of the PHP graphql library, we can const it but I don't think they expose a const for it (silly random associative arrays)
@@ -55,18 +55,30 @@ | ||
*/ | ||
class FederatedSchema extends Schema |
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.
The docstring for the class has not been updated to include this new functionality. That's not required, but would be useful
protected ServiceDefinitionType $serviceDefinitionType; | ||
protected EntityUnionType $entityUnionType; | ||
protected AnyType $anyType; | ||
|
||
public function __construct($config) |
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.
What is the signature of the config array? It didn't exist before but it certainly can now, especially as we introduce even more properties.
if ($type instanceof EntityObjectType) { | ||
$entityTypes[$type->name] = $type; | ||
return function () use ($config) { | ||
$resolvedTypes = TypeInfo::extractTypes($config['query']); |
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.
More magic strings for the config, +1 for annotating that config array
]); | ||
} | ||
return self::$overRiddedEpisodesSchema; | ||
} | ||
|
||
public static function getTypeLoader(): callable { |
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 know it's just a test, but maybe we should consider using something like containers for our test/example implementation. Using something like an array even would let us register these and look them up in a straightforward way.
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.
Or, we simply break out an example usage separate from the test. But it feels like the test suite is actually perfect for demonstrating an example usage in this case.
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.
Feel free to 👎
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 think as it is it's okay. I think if this keeps growing though we will need some sort of TypeRegistry you're right.
Co-authored-by: Tom Carrio <tom@carrio.dev>
'types' => is_callable($entityTypes) | ||
? fn () => array_values($entityTypes()) | ||
: array_values($entityTypes) |
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.
Starting to see this pattern often enough. What we have is essential a type like MaybeLazyEntityTypes = T | () => T
. We can define a common utility for working with this.
function normalizeEntityTypes($maybeLazyEntityTypes): array {
return is_callable($maybeLazyEntityTypes)
? $maybeLazyEntityTypes()
: $maybeLazyEntityTypes;
}
Which you can use as such:
'types' => is_callable($entityTypes) | |
? fn () => array_values($entityTypes()) | |
: array_values($entityTypes) | |
'types' => array_values(normalizeEntityTypes($entityTypes)), |
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.
we need to preserve the lazy nature of the operation
So we need to wrap the operation in a new lazy operation that then updates it on invocation
Which is not the same as invoking it and operating on the result.
So alas we cannot commonize this
Proposed changes
typeLoader
config option by exposing the types added for the FederatedSchema (namely,_Service
,_Entity
,_Any
EntityUnionType
from doing an entire type tree scantypes
field a callable so that theoretically the underlying entity types (and therefore the type scan) only happens on analysis/interpretation of the_Entity
type.How to test
Unit Tests