Remember initial types instead of lazy-loading

[rudiedirkx]

Types that are given in schema config 'types' don't have to be 'typeLoader' loaded later:

new Schema([
	'typeLoader' => function(string $name) {
		return GraphQLFactory::type(sprintf('App\Services\GraphQL\Schema\%sType', $name));
	},
	'query' => GraphQLFactory::type(ClubQueryType::class),
	'mutation' => GraphQLFactory::type(ClubMutationType::class),
	'types' => [
		GraphQLFactory::type(ReservationInterfaceType::class),
		GraphQLFactory::type(ClassReservationType::class),
		GraphQLFactory::type(CourtReservationType::class),
	],
]);

The 3 provided types don't have to go through the typeLoader. In my case the type loader doesn't work for those 3, but everything else should be lazy loader by typeLoader.

This might break something else, but I'm curious to see all the tests.

[rudiedirkx]

Current workaround:

$types = [
	'Datetime' => DatetimeType::class,
	'ReservationInterface' => ReservationInterfaceType::class,
];
return new Schema([
	'typeLoader' => function(string $name) use ($types) {
		$typeClass = $types[$name] ?? sprintf('App\Services\GraphQL\Schema\%sType', $name);
		return GraphQLFactory::type($typeClass); // @phpstan-ignore argument.templateType
	},
	'query' => GraphQLFactory::type(ClubQueryType::class),
	'mutation' => GraphQLFactory::type(ClubMutationType::class),
]);

but I rather put those $types types in Schema's 'types'.

[spawnia]

Users may specify types in a multitude of ways, its type is iterable<Type&NamedType>|(callable(): iterable<Type&NamedType>)|iterable<(callable(): Type&NamedType)>|(callable(): iterable<(callable(): Type&NamedType)>). The code you wrote does not work with some of those options.

[rudiedirkx]

I could fix this by maybe calling ->types and $type, BUT that negates the lazyness of the callables, so maybe my approach is wrong? Maybe Schema should only do this if types AND typeLoader are given? Or not, since without typeLoader, the entire schema is loaded anyway. Maybe my 'workaround' is the right way to do conservative type-loading? You tell me :)

[rudiedirkx]

I don't like this fix at all. Copied from getTypeMap. That's a lot of callback and @var and assert(). Maybe my approach is just wrong. How to pre-load types better?

[Copilot]

Pull request overview

This pull request attempts to optimize schema initialization by eagerly resolving types provided in the types configuration option during the Schema constructor, avoiding the need to load them later through the typeLoader. The intention is to allow some types to bypass the typeLoader mechanism when they are explicitly provided upfront.

Changes:

• Added code in the Schema constructor to immediately resolve all types from the types config and store them in $this->resolvedTypes
• Types are resolved by calling resolveType() on each type or lazy type in the array
• Includes a commented-out line suggesting config.types should be cleared after resolution

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The assert statement assumes all types in the 'types' config are NamedType instances. However, if a callable in the types array returns a non-NamedType (e.g., a wrapper type), the assertion will fail in development but will pass silently in production (where assertions are typically disabled). This could lead to undefined behavior. Consider replacing the assert with a proper type check and throwing an InvariantViolation if the type is not a NamedType.

[Copilot]

This commented-out line suggests uncertainty about whether types should be cleared from the config after pre-loading. However, leaving the types in the config means they will be resolved again in getTypeMap() (lines 130-151), which could cause issues with type identity checks and performance. The PR description mentions that this "might break something else", indicating this is an incomplete solution. Either this line should be uncommented with proper testing, or the overall approach needs reconsideration.

Suggested change

	// $this->config->types = []; // Don't resolve these again?
	$this->config->types = []; // Don't resolve these again.

[Copilot]

Eagerly resolving all types in the constructor defeats the purpose of lazy loading and can negatively impact performance for large schemas. If the types array contains many types or lazy callables, all of them will be resolved immediately during schema construction, even if they're never used. This is particularly problematic when combined with typeLoader, as it changes the expected behavior where types are only loaded on-demand. Consider documenting this behavior change and its performance implications.

Suggested change

	if (is_callable($types)) {
	$types = $types();
	}
	foreach ($types as $typeOrLazyType) {
	/** @var Type|callable(): Type $typeOrLazyType */
	$type = self::resolveType($typeOrLazyType);
	assert($type instanceof NamedType);
	$this->resolvedTypes[$type->name()] = $type;
	}
	// If $types is a callable, defer its execution to preserve lazy loading.
	if (! is_callable($types)) {
	foreach ($types as $typeOrLazyType) {
	/** @var Type|callable(): Type $typeOrLazyType */
	$type = self::resolveType($typeOrLazyType);
	assert($type instanceof NamedType);
	$this->resolvedTypes[$type->name()] = $type;
	}
	}

[Copilot]

Resolving types eagerly in the constructor can trigger side effects and field resolution callbacks prematurely. Many types use lazy field resolution (via callables) to avoid circular dependencies. By calling resolveType() on all types in the constructor, any lazy types with closures will be invoked immediately, potentially causing errors if they reference types that haven't been fully initialized yet. This is especially problematic for schemas with circular type dependencies.

Suggested change

	$types = $this->config->types;
	if (is_callable($types)) {
	$types = $types();
	}
	foreach ($types as $typeOrLazyType) {
	/** @var Type|callable(): Type $typeOrLazyType */
	$type = self::resolveType($typeOrLazyType);
	assert($type instanceof NamedType);
	$this->resolvedTypes[$type->name()] = $type;
	}
	// $this->config->types = []; // Don't resolve these again?
	// Types from $this->config->types are intentionally not eagerly resolved here.
	// They may include lazy callables and are resolved on demand elsewhere
	// (for example, when building the type map).

[Copilot]

When both types and typeLoader are provided together (as shown in ExecutorLazySchemaTest.php:85-107), this change could lead to type instance conflicts. If the typeLoader returns different instances than what was pre-resolved in the constructor, it violates the "same instance" requirement enforced by assertValid() at line 532-534. The existing tests expect that types are only resolved when needed via typeLoader, but this change forces immediate resolution of types array, potentially creating mismatched instances.

Suggested change

	$types = $this->config->types;
	if (is_callable($types)) {
	$types = $types();
	}
	foreach ($types as $typeOrLazyType) {
	/** @var Type|callable(): Type $typeOrLazyType */
	$type = self::resolveType($typeOrLazyType);
	assert($type instanceof NamedType);
	$this->resolvedTypes[$type->name()] = $type;
	}
	// $this->config->types = []; // Don't resolve these again?

[Copilot]

The types pre-loaded in the constructor will be lost when getTypeMap() is called. At line 136, getTypeMap() resets $this->resolvedTypes = [], which will clear all types that were pre-resolved in the constructor. This means types provided in the 'types' config will still be lazily loaded through the typeLoader, defeating the purpose of this change.

To fix this, either:

1. Store the pre-loaded types separately and merge them after the reset in getTypeMap()
2. Modify getTypeMap() to preserve types that were pre-loaded in the constructor
3. Conditionally skip the reset if types were already loaded

Suggested change

	$types = $this->config->types;
	if (is_callable($types)) {
	$types = $types();
	}
	foreach ($types as $typeOrLazyType) {
	/** @var Type|callable(): Type $typeOrLazyType */
	$type = self::resolveType($typeOrLazyType);
	assert($type instanceof NamedType);
	$this->resolvedTypes[$type->name()] = $type;
	}
	// $this->config->types = []; // Don't resolve these again?

[Copilot]

The constructor does not validate for duplicate type names, but getTypeMap() does (line 145-148). This means duplicate types in the 'types' config will silently overwrite each other in the constructor, potentially causing unexpected behavior. The validation check from getTypeMap() should be added to the constructor loop as well.

Suggested change

	$this->resolvedTypes[$type->name()] = $type;
	$typeName = $type->name();
	if (isset($this->resolvedTypes[$typeName]) && $this->resolvedTypes[$typeName] !== $type) {
	throw new InvariantViolation(
	'Schema must contain unique named types but contains multiple types named ' . $typeName
	);
	}
	$this->resolvedTypes[$typeName] = $type;