feat: implement tristate optionals (some, null, undefined)#1149
feat: implement tristate optionals (some, null, undefined)#1149definitelynobody wants to merge 2 commits intoobmarg:mainfrom
Conversation
✅ Deploy Preview for cynic-querygen-web canceled.
|
definitelynobody
force-pushed
the
maybe_undefined
branch
from
6351df3 to
4c20a96
Compare
|
This is mostly working but I'm making this a draft until I've tested it enough. |
| rust_decimal = "1.22" | ||
| thiserror = "1.0.30" | ||
| uuid = { version = "1", features = ["v4"] } | ||
| async-graphql = { version = "7" } |
There was a problem hiding this comment.
async-graphql is a very heavy dependency to pull in, particularly just for a single 3 variant enum. Can you implement this without the dependency please.
| is_subobject_with_lifetime, | ||
| ) | ||
| .map(|type_spec| format!("Option<{type_spec}>",)); | ||
| .map(|type_spec| format!("cynic::MaybeUndefined<{type_spec}>",)); |
There was a problem hiding this comment.
I don't want to force every user of the generator into using MaybeUndefined. I'd rather this was controlled by a parameter somewhere - either a parameter passed in rust or maybe a directive in the query that we're generating from?
| impl<T, TypeLock> CoercesTo<MaybeUndefined<TypeLock>> for Option<T> where T: CoercesTo<TypeLock> {} | ||
| impl<T, TypeLock> CoercesTo<MaybeUndefined<TypeLock>> for MaybeUndefined<T> where | ||
| T: CoercesTo<TypeLock> | ||
| { | ||
| } |
There was a problem hiding this comment.
Are you sure these CorecesTo<MaybeUndefined<... impls are needed? I feel like they might not be....
| impl<T> QueryFragment for MaybeUndefined<T> | ||
| where | ||
| T: QueryFragment, | ||
| { | ||
| type SchemaType = Option<T::SchemaType>; | ||
| type VariablesFields = T::VariablesFields; | ||
|
|
||
| fn query(builder: SelectionBuilder<'_, Self::SchemaType, Self::VariablesFields>) { | ||
| T::query(builder.into_inner()) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not really sure is there's a use case for MaybeUndefined as a QueryFragment. It makes sense for input types but not so much for output types which will never be undefined in a well formed graphql response....
| /// - `From<Option<T>>` will become T or null. | ||
| /// - `Default` will become undefined. | ||
| #[derive(Serialize, Deserialize, Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug, Hash)] | ||
| pub struct MaybeUndefined<T>(async_graphql::MaybeUndefined<T>); |
There was a problem hiding this comment.
As I already mentioned can you implement MaybeUndefined here rather than pulling in async_graphql
| impl<T, U> IsScalar<MaybeUndefined<T>> for MaybeUndefined<U> | ||
| where | ||
| U: IsScalar<T>, | ||
| { | ||
| type SchemaType = MaybeUndefined<U::SchemaType>; | ||
| } |
There was a problem hiding this comment.
Pretty sure this impl isn't needed
|
Thanks for the review I will take a look at this in a few days |
2 participants
Fixes #125
Why are we making this change?
This allows for specifying a little extra information for optional types (some, null, default) instead of Option which cannot provide a default value.
In my use case I run the codegen in a build script. I don't want to tweak my input objects at all. Having this type makes my code more flexible.
What effects does this change have?
Creating an cynic::MaybeUndefined type which behaves a lot like option, but wraps async_graphql::MaybeUndefined and currently forwards that type. It might be a good idea to not wrap that type if we are worried about increasing the API surface or compile times or something else.
You can still use Option. The codegen will use MaybeUndefined instead and includes
skip_serializing_ifto exclude the value when it shouldn't be serialized.