Skip to content

Support custom scalar types #232

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

Closed
weiznich opened this issue Aug 24, 2018 · 6 comments
Closed

Support custom scalar types #232

weiznich opened this issue Aug 24, 2018 · 6 comments
Labels
discussion enhancement Improvement of existing features or bugfix

Comments

@weiznich
Copy link
Contributor

I've toyed a bit around to support custom scalar values in juniper.
The following is some sort of brain dump of the current state.

Adding support for new scalar types that are mapping to existing types seems to be quite easily possible, by reusing existing enum variants in Value and InputValue. Using this approach it is for example possible to add support for a 16 bit integer type, because we could simply map that to the existing 32 bit integer. On the other hand adding support for types that are not representable by the existing enum variants is not possible. (An Example here would be adding support for 64 bit integers. They would potentially cause overflows in the 32 bit integer enum variant)

I see several ways to "solve" this: (I've not tried any of this so it may work or it may not work)

  • Add more variants to both enums. So that there is a own variant for each supported type.
    This approach has several downsides:

    • Only a limited set of types could be supported, so if someone want's to introduce a fancy type doing crazy things he will hit the same limit as today
    • We need to teach the parser when to use which integer type (I think this information should already exist somewhere in the schema)
  • Use other types in the existing enum variants
    Again there are downsides:

    • What's the correct type to uses? For integers: should we use i64, but what when someone want's to represent a i128 and so on
    • Again, this is not extensible by some crazy new user defined type
  • Try to make the schema generic about the exact Value/InputValue type, provide a default Value type (maybe the existing one) and allow users to override this type
    Downsides:

    • Again we need to use more knowledge from the schema in the parser
    • I would guess this involves at least rewriting bigger parts of the schema and the parser

I would love to here some other opinions on this before trying further to make this work in a nice way.

(This is basically the only remaining open point for wundergraph)

cc @theduke @LegNeato

Related: #198

@LegNeato
Copy link
Member

LegNeato commented Aug 27, 2018

Thanks for the investigation in this and #198 !

I don't know why, but this still seems off to me. It feels like leaking the implementation details / server language semantics into the GraphQL layer. For example, a JS client wouldn't care about i16 vs i32, yet it now needs to care because the server exposes both as scalar types.

Then again, custom scalar types are essentially the server telling the client what the requirements are and the invariants that hold for that field, so maybe it is the right thing to do.

I guess if we define these types in juniper consumers of the lib aren't forced to use them. They could just use the default Int and Float types and it would be as if they were never added.

So, I think I adding these via a feature flag that defaults to on--similar to the uuid, url, etc integrations--would be a good way forward. That way, if juniper users want to stick to only the scalars defined in the spec and defined by their code they can.

That being said, it sounds like we aren't really set up to handle this case based on your investigations, right? We can treat them as a String but when it hits the Juniper machinery we can't really support things like i128 because at the end of the day the "core" needs to pass a typed value around and there isn't a great way to know in advance what that type should be in the custom scalar case.

@LegNeato LegNeato added enhancement Improvement of existing features or bugfix discussion labels Aug 28, 2018
@LegNeato
Copy link
Member

Also see graphql/graphql-spec#326

@weiznich
Copy link
Contributor Author

I don't know why, but this still seems off to me. It feels like leaking the implementation details / server language semantics into the GraphQL layer. For example, a JS client wouldn't care about i16 vs i32, yet it now needs to care because the server exposes both as scalar types.

From the perspective of a JS client this is looking like leaking a implementation detail, right. But who says that the Client needs to be JS (and also the transport layer needs to be http with json serialization.)
As far as I understand GraphQL it is not (at least) entirely a thing purely build for webapps but could also be used in other places. So let's be a bit creative and construct some use cases that would for example benefit from having different integer types:

  • Using a different serialisation (for example bincode instead of json)
  • Using a js dialect or a language that compiles to wasm/asm.js for the frontend that can handle
  • … (More creative examples 😉)

I guess if we define these types in juniper consumers of the lib aren't forced to use them. They could just use the default Int and Float types and it would be as if they were never added.

If someone does not (want) to use those types they should not be seen anywhere.

So, I think I adding these via a feature flag that defaults to on--similar to the uuid, url, etc integrations--would be a good way forward. That way, if juniper users want to stick to only the scalars defined in the spec and defined by their code they can.

That being said, it sounds like we aren't really set up to handle this case based on your investigations, right? We can treat them as a String but when it hits the Juniper machinery we can't really support things like i128 because at the end of the day the "core" needs to pass a typed value around and there isn't a great way to know in advance what that type should be in the custom scalar case.

After a bit toying around I think the proper solution is to replace the scalar enum variants in Value and InputValue with something generic and let the user decide what exact representation for scalar values should be used.
I've started to implement that here but unfortunately rustc does not like me and eats all my memory 😟

@LegNeato
Copy link
Member

It looks like the spec might be changing soon: graphql/graphql-spec#521

@weiznich
Copy link
Contributor Author

It looks like the spec might be changing soon: graphql/graphql-spec#521

As far as I understood this proposal this only adds a way to specify that a custom scalar value has the same representation as an existing scalar value. This does not forbid doing something like type BigInt and then using a i64 there. So fundamentally #251 continues to be "required".

@weiznich
Copy link
Contributor Author

This should be fixed after #251 was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement Improvement of existing features or bugfix
Projects
None yet
Development

No branches or pull requests

2 participants