astFromValue fails with a custom scalar serializing to an object value

[ardatan]

When you have a scalar with a serialize function that returns an object, it is not possible to convert it to AST. So it doesn't allow you to print a schema that has an input value with an object as a default value.

Reproduction -> https://github.com/graphql/graphql-js/pull/4086 Proposed fix -> https://github.com/graphql/graphql-js/pull/4087

const JSONScalar = new GraphQLScalarType({
  name: "JSON",
  serialize(value) {
    return value;
  },
});

const schema = new GraphQLSchema({
  types: [JSONScalar],
  query: new GraphQLObjectType({
    name: "Query",
    fields: {
      test: {
        type: GraphQLString,
        args: {
          i: {
            type: JSONScalar,
            defaultValue: { object: true },
          },
        },
      },
    },
  }),
});

[yaacovCR]

I think this would be solved alternatively by: https://github.com/graphql/graphql-js/pull/3814 .

The relevant code bit would be:

    const literal =
      arg.defaultValue.literal ??
      valueToLiteral(arg.defaultValue.value, arg.type);

where the first condition reads the literal from the preserved literal if the schema was created from an AST, and the second bit converts the value to a literal in a type-safe manner if the schema was created programattically.

see https://github.com/yaacovCR/graphql-executor/blob/57e32d78041e0263991b7ae2488c62fde32ae930/src/utilities/printSchema.ts#L261-L263

[ardatan]

So your PR introduces a new method in the leaf types called valueToLiteral right? Looks more elegant actually. But your PR looks a big change. Maybe we can try to fix this area specifically first before a huge refactor. What do you think?

[ardatan]

There is also this issue which is a bit related; https://github.com/graphql/graphql-js/pull/4088 @yaacovCR I am curious what do you think?

[yaacovCR]

Yes I think it should also be covered => custom scalars may have to define custom valueToLiteral methods

just to be clear, while my name is on the rebased PR, all the work is from @leebyron (and the feedback he received from others, of course) — my role has just been trying to keep it alive by rebasing it periodically.

I hope it eventually gets in!

[dotansimha]

@yaacovCR I agree it will be solved by https://github.com/graphql/graphql-js/pull/3814 , but the seems like a bigger refactor.

I think there's room for improvement in the current impl, without refactoring it. I tend to agree with @ardatan 's take that this seems like a bug that should be fixed.