Convert arbitrary scalar values into ASTs and back

[anhldbk]

Reporting issues with GraphQL.js

This issue is related to: https://github.com/graphql/graphql-js/issues/1815

Expected:

• Successfully convert arbitrary scalars (including default values) to ASTs
• Convert ASTs into dedicated values

Code:

const {
  makeExecutableSchema,
} = require('graphql-tools')

const {
  astFromValue,
  valueFromAST,
  isValidJSValue
} = require('graphql/utilities')

const {
  GraphQLScalarType
} = require('graphql')

const JsonScalarType = new GraphQLScalarType({
  name: 'JSON',
  serialize: (value) => value,
});

const resolveFunctions = {
  JSON: JsonScalarType
};

const typeDefs = `
  scalar JSON
  input Message {
    extra: JSON
    meta: JSON = {}
  }
`

const schema = makeExecutableSchema({
  typeDefs,
  resolvers: resolveFunctions
})

const messageType = schema.getType('Message')

const value = {
  extra: {
    'key': 'andy',
    "kafka.producer.batch": 0
  }
}

// check if there's any error in our value
const errors = isValidJSValue(value, messageType)
// errors will contain detail errors if any
console.log(`Valid: ${errors.length==0}`)

// parse and get value 
const ast = astFromValue(value, messageType)
const conf = valueFromAST(ast, messageType)

Result

$ npm start                                                                                                              master ✱ ◼

> gql@1.0.0 start /Users/andy/Works/gql
> babel-node --presets es2015 run.js

Valid: true
/Users/andy/Works/gql/node_modules/graphql/utilities/astFromValue.js:130
          throw _iteratorError;
          ^

TypeError: Cannot convert value to AST: { key: "andy", kafka.producer.batch: 0 }
    at astFromValue (/Users/andy/Works/gql/node_modules/graphql/utilities/astFromValue.js:193:11)
    at astFromValue (/Users/andy/Works/gql/node_modules/graphql/utilities/astFromValue.js:107:26)

Solution

I have to patch astFromValue: https://github.com/graphql/graphql-js/blob/8aef229cb2/src/utilities/astFromValue.js#L139-L140 by adding following lines before L139

if (typeof serialized === 'object' ){
    return {
      kind: _kinds.Kind.OBJECT,
      value: serialized
    };
}

Question

The solution above may not be adequate. Would you please tell me it's worth to make a RP against this issue and how to properly resolve it?

[anhldbk]

@IvanGoncharov FYI

[IvanGoncharov]

@anhldbk Thanks for detail report. Also, note that isValidJSValue is deprecated and will be removed in upcoming 15.0.0: https://github.com/graphql/graphql-js/blob/b8a0f3b9a9fc7057acb8e91327818b37f0eaf406/src/utilities/isValidJSValue.js#L14-L22

I have to patch astFromValue: https://github.com/graphql/graphql-js/blob/8aef229cb2/src/utilities/astFromValue.js#L139-L140 by adding following lines before L139

It's a little bit more complex, correct AST node for object value should contain fields instead of value: https://github.com/graphql/graphql-js/blob/b8a0f3b9a9fc7057acb8e91327818b37f0eaf406/src/language/ast.js#L346-L350

I think correct solution would be introduce serializeAsLiteral (opposite to parseLiteral) and make it default to astFromValueUntypped(this.serialize(value)) (opposite to valueFromASTUntyped).

[anhldbk]

@IvanGoncharov So when users define a custom scalar, they must provide a function named serializeAsLiteral?

[IvanGoncharov]

@anhldbk It's designed proposal for a fix. Idea is to have optional serializeAsLiteral that if omitted equals to astFromValueUntypped(this.serialize(value)).

[vitramir]

I think this is a better fix:

    if (typeof serialized === 'object') {
      if (Array.isArray(serialized)) {
        return {
          kind: Kind.LIST,
          values: serialized.map(v => astFromValue(v, type)),
        };
      }

      return {
        kind: Kind.OBJECT,
        fields: Object.entries(serialized).map(([k, v]) => {
          return {
            kind: 'ObjectField',
            name: { kind: 'Name', value: k },
            value: astFromValue(v, type),
          };
        }),
      };
    }

I think we can keep Enum and GraphQLID logic in this function and move all the stuff regarding scalars (boolean, string, number and this fix also) to astFromValueUntyped function.

@IvanGoncharov Do you want me to make PR for these changes?

[andimarek]

just fyi: GraphQL Java will add a very similar method to the proposed serializeAsLiteral. (cc @bbakerman)

https://github.com/graphql-java/graphql-java/pull/1745