Skip to content
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

astFromValue fails with a custom scalar serializing to an object value #4085

Open
ardatan opened this issue May 8, 2024 · 5 comments · May be fixed by #4087
Open

astFromValue fails with a custom scalar serializing to an object value #4085

ardatan opened this issue May 8, 2024 · 5 comments · May be fixed by #4087

Comments

@ardatan
Copy link
Member

ardatan commented May 8, 2024

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 -> #4086
Proposed fix -> #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
Copy link
Contributor

yaacovCR commented May 8, 2024

I think this would be solved alternatively by: #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
Copy link
Member Author

ardatan commented May 8, 2024

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
Copy link
Member Author

ardatan commented May 8, 2024

There is also this issue which is a bit related;
#4088
@yaacovCR I am curious what do you think?

@yaacovCR
Copy link
Contributor

yaacovCR commented May 8, 2024

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
Copy link
Member

@yaacovCR I agree it will be solved by #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants