Skip to content

Conversation

@oprypkhantc
Copy link
Contributor

@oprypkhantc oprypkhantc commented Mar 10, 2023

Closes #573

On the implementation: I've implemented VoidTypeMapper separately from the rest to make sure it only ever matches standalone output types, i.e. that it can't be used in a union or as an input or anything else that doesn't make sense. NullableTypeMapper is no longer the "top root type mapper" - that's because void type must not be wrapped in NonNull as that causes an error when actually running a mutation with void return.

@oprypkhantc oprypkhantc marked this pull request as ready for review March 10, 2023 22:38
@@ -0,0 +1,52 @@
<?php
Copy link
Collaborator

@oojacoboo oojacoboo Mar 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I'm a little confused on the purpose of the src/Types.php file. It seems like you're creating this as a means of separating out the GraphQLite "custom-ish" types. But, I don't really see how these differ much from the other types in the Types dir. They're all internally managed types and pulling out a few seems a bit confusing.

Is the internal caching here providing much value? The instantiation of these objects seems trivial.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is similar to webonyx's Type class with string, id, getNamedType, union etc method, i.e. if you need a type you know right away where to get it from. The value of Types in this case is the same kind of "entry point" for those who need those types in 3rd party type mappers. I'm not worried about caching so much.

For example, in our project we use carbon/carbon instead of php's DateTime, but we still use graphqlite's built in DateTime type to represent Carbon dates it in the Schema and serialize/deserialize them. When more types are added into graphqlite, it will always be easy to navigate and find them.

If you're against providing that kind of public API I'll remove it.

Copy link
Collaborator

@oojacoboo oojacoboo Mar 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. For DateTime, we use a custom type mapper for our internal Date object that maps to a custom Date scalar. We do allow GraphQLite to handle \DateTime and \DateTimeImmutable. We have a few other custom types as well that we manage through our type mapper.

Carbon extends \DateTimeImmutable IIRC. I think GraphQLite should handle the serialization of this already. Of course, you could setup a type mapper to handle the deserialization side as well.

I'm trying to think where this API would even be helpful to a normal developer. And internally, I don't see much benefit over using the type classes directly. You can't type with this interface, requiring the class to be typed directly. I think I'd rather we stick with the type classes directly, for the sake of simplicity and consistency.

Copy link
Contributor Author

@oprypkhantc oprypkhantc Mar 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed Types. Following the style of BaseTypeMapper, I still used a static property to hold an instance of VoidType, just to avoid the unnecessary instantiations.

@oojacoboo oojacoboo merged commit 5a9c03b into thecodingmachine:master Mar 13, 2023
@oprypkhantc oprypkhantc deleted the void-type branch March 13, 2023 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

First party support for void return type

2 participants