Graphql-js: simplify custom scalars

[mattkrick]

I've seen some confusion with parseValue and parseLiteral (I know I've seen some bad example & promulgated those bad habits with code I wrote in OSS land 🙈 ).

After playing around with a bunch of different types, I think we could make the 2 easier:

• parseLiteral _only_ receives & handles ast.kind
• parseValue _only_ receives & handles values, like ast.value
• Things that currently call parseLiteral should now call both

So for example, the GraphQLFloat:

  parseValue: (value) => {
    if (!value && value !== 0) {
      throw new TypeError(
        'Float cannot represent non numeric value: (empty string)'
      );
    }
    const num = Number(value);
    if (num === num) {
      return num;
    }
    throw new TypeError(
      'Float cannot represent non numeric value: ' + String(value)
    );
  },
  parseLiteral(kind) {
    if (kind !== Kind.FLOAT && kind !== Kind.INT) {
      throw new TypeError('Bad kind') 
    } 
  }

Doing this allows for each method to validate only 1 piece (kind or value). That enables code reuse. Right now, if folks want to use the same logic, they have to create a mock AST in parseValue: https://github.com/stylesuxx/graphql-custom-types/blob/master/src/types.js#L13-L17. Assuming you're on board & OK with a breaking change, we should probably rename parseLiteral to validateKind or something since all parsing would be handled by 1 function.

Now call me crazy, but I think while we're at it, we could also have serialize default to parseValue, since the two are identical for everything except things that aren't supported by JSON (like Dates and.... maybe that's it?).

[leebyron]

I actually think this could be problematic and make these bad habits worse. Instead I'd prefer to see fewer custom scalars created in the first place - well designed APIs typically have very few of them.

parseValue and parseLiteral are quite similar as you've seen, but because the inputs types are different, the parsing behavior may be different. If there happens to be code reuse that could be pulled into a separate function, that's totally worthwhile, however I don't think the API should _demand_ such things because it might not always be possible.

serialize and parseValue must be different because they often encode different rules. For example some scalars might be required to only be used in the input position (serialize should throw?) or in order to maintain a strictly typed API but allow return types to be coerced.

You can see examples of both of these in the built-in scalars in this repo

[stubailo]

Right now it's not very clear what custom scalars are supposed to be used for. The primary use case I see in the community is actually for validation - having custom scalar input types that validate when parsing. There are definitely some issues with the approach, but there isn't any other way of declaratively specifying validation, other than putting it at the beginning of the resolver function.

[mattkrick]

Agreed, I see this disconnect between a GraphQL schema & a validation schema (eg joi, yup, etc).
Let's say I have a name field that must be between 3 and 100 characters.

• I validate it on the client so the user gets a friendly, synchronous message
• I validate it on the server so some punk doesn't sneak in a 30MB string
• GraphQL validates that the string is a string... 🎉

Assuming a node backend, there's no reason I couldn't use custom scalars to solve all 3. Use custom scalars to do more fine-grained validation on the server. Use a webpack plugin to pluck parseValue from each type and send that to the client. That seems really beautiful to me.

If you say that using fine-grained custom scalars is a bad idea, I've gotta trust you because you've been doing this a lot longer than I have, but from here, it looks pretty sweet. Alternatively, I could use joi/yup to write a validation schema and share that across server/client, but that means manually keeping the validation schema up to date with the GraphQL schema. Any insights on how facebook does it would be super interesting!

[leebyron]

@stubailo - that's a great point. I think it would be worthwhile to consider other APIs for validating inputs, I'm certainly open to ideas on that front. I expected custom scalars to be used for defining semantically different types like URL or DateTime.

I hadn't anticipated a comparison between validation schemas and GraphQL - instead I thought of it more like a language type system where it's really not common to create variants of String that only allow providing values of certain lengths or that match some regex.

At Facebook we're doing our input validation local to their usage. Typically this comes in two flavors: for the read API, we're really doing very simple input validation - often times just making sure your int for an image size is a positive and non-zero. The other flavor is for mutations, and there we often have validations that are decidedly not serializable - they're arbitrary business logic and even sometimes require fetching information to determine (so they're async as well). In practice serializable regex-style validation isn't sufficient for this kind of thing.

This can make it hard to share this kind of validation directly with clients for getting the friendly sync messages, but also in well designed UI those aren't just message text to display, but far more often some specific UI behavior or animation, which isn't really something GraphQL is designed to express.

[mattkrick]

@leebyron @stubailo thanks for the thoughtful responses, lots of good stuff to dig in on!
Currently, I've been basing UI behavior on serializable outcomes. Eg
{_error: 'User Creation Failed', lastName: 'Last name cannot be empty'} tells me the overarching error, and the specific field, and then that can trigger animations.
You're absolutely right that using GraphQL as a validating schema falls flat for contextual validation (if field 1 is blank, field 2 must have something). I try to avoid that stuff, but if I really needed it, I could still use GraphQL as my base schema (via webpack) & build on top of it with a library like yup.
For async validation, even validation libraries can't handle it, so I'm less concerned with that being a limiting factor as I'd have to handle that outside the schema, anyways.

An API that breaks apart validation & resolution would be fantastic, but I'm not sure if that line is always black & white. I have some mutations that will grab some data, validate it, then grab some more based on the previous validation result, so breaking it apart would mean eager or redundant validation.

Happy to close the issue for now but extra conversation is always welcome. thanks for the input!