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

Scalar's serialize, parseValue and parseLiteral #500

Closed
adriano-di-giovanni opened this issue Sep 22, 2016 · 12 comments
Closed

Scalar's serialize, parseValue and parseLiteral #500

adriano-di-giovanni opened this issue Sep 22, 2016 · 12 comments

Comments

@adriano-di-giovanni
Copy link

Please, correct me if I'm wrong

serialize gets invoked when we query data (i.e. we format a Date from MongoDB to ISO 8601)
parseValue gets invoked when we mutate data (i.e. we get a value from an InputType and try to coerce it to a Javascript value.

What about parseLiteral?

Thanks

@helfer
Copy link
Contributor

helfer commented Sep 22, 2016

serialize: gets invoked when serializing the result to send it back to a client.

parseValue: gets invoked to parse client input that was passed through variables.

parseLiteral: gets invoked to parse client input that was passed inline in the query.

@leebyron
Copy link
Contributor

Correct.

parseValue gets a plain JS object, and parseLiteral gets a value AST.

@adriano-di-giovanni
Copy link
Author

Sorry if I'm re-opening this but I'm working again on scalars and there's something I can't understand event reading your code:

  • should serialize always return a Stringvalue?
  • should parseValue and parseLiteral always return the same value?

Thanks,
Adriano

@amir-s
Copy link

amir-s commented Jan 6, 2017

@adriano-di-giovanni I know it is from long time ago. I came here by search, so I thought I might answer.

should serialize always return a String value?

No. The response in GraphQL is JSON. So, you could return anything that can be represented as such. Number, String, Array or simply JS Object.

should parseValue and parseLiteral always return the same value?

Yes, I believe. In a resolver function, we don't care whether the value of a type was inline or came from variables object. It should be the same. However since there are two ways of reading the input and the type of the input are different in those two ways, we need two methods for them.

Also : http://stackoverflow.com/questions/41510880/whats-the-difference-between-parsevalue-and-parseliteral-in-graphqlscalartype/41513681#41513681

@pscanf
Copy link

pscanf commented Jan 21, 2017

should serialize always return a String value?

No. The response in GraphQL is JSON. So, you could return anything that can be represented as such. Number, String, Array or simply JS Object.

Looking at the src/utilities/astFromValue.js it seems like serialize  can only return nullish values, booleans, numbers or strings.

This constraint doesn't seem to be enforced during serialization though, so problems occur only when astFromValue is invoked, ie during introspection.

Reading around the spec and the source code I wasn't able to figure out what's the intended behaviour though. Should scalars ultimately resolve to either ~nulls, booleans, numbers or strings?

@shtse8
Copy link

shtse8 commented Oct 29, 2017

Sorry, I still don't understand. Should we implement two methods or just only one of them?

@mrdulin
Copy link

mrdulin commented Jul 31, 2018

@helfer thx. I am clear

@timqian
Copy link

timqian commented Oct 3, 2018

Don't get the API design purpose too. If the parseValue and parseLiteral should return the same value, why exposing two functions? Isn't this should be handled by the library instead of letting the user write the same logic twice?
Does anyone has any example when we might need diffrenet beheavior for these two api?

@amir-s
Copy link

amir-s commented Oct 3, 2018

@timqian you can refer to my answer on stackoverflow.

The input for those two methods are different. One receives AST, the other one receives parsed javascript object (if you want to call it JSON).

@livingmine
Copy link

I'm also curious by the API design purpose just like @timqian , if someone could elaborate more about why this should be handled by the library's user instead of the library itself, that would be great! However, there is the astFromValue helper function though, so one can provide a single logic only on the parseLiteral and simply pass the value from the parseValue to the helper first then pass it to the parseLiteral.

@IvanGoncharov
Copy link
Member

Maybe I'd understand better if there was an example query on a custom scalar that showed how a single parse function would be insufficient.

Disclaimer: Maybe not the best example but it's something I come up with in 3AM after I returned back from pub 🍻

@boostwilliam For example, let's take Int64 as an example (standard GraphQLInt supports only 32 bit).
JSON.parse doesn't support such values:

JSON.parse('18446744073709551616')
// 18446744073709552000

So your only option is to send such values as strings.
At the same time GraphQL support arbitirary long numbers so you can write:

{
  foo(arg: 18446744073709551616)
}

Our goal is to write parseLiteral/parseValue that will return BigInt

new GraphQLScalar({
  name: 'Int64',
  parseLiteral(ast) {
    if (ast.kind !== Kind.INT) {
      throw Error('...');
    }
    return BigInt(ast.value);
  }
  parseValue(value) {
    if (typeof value !== 'string') {
      throw Error('...');
    }
    return BigInt(value);
  }
})

It's just one example from the top of my head there are a few other reasons for why parseLiteral/parseValue should be defined as separate functions.

@IvanGoncharov
Copy link
Member

I was thinking about this issue for the last few months and I agree that in the majority of cases you don't need separate parseLiteral function and it just leads to unnecessary code duplication.
That said as I showed in my previous post, having the ability to write custom parseLiteral is critical in some scenarios.
In #1955 I tried to resolve this conflict by making parseLiteral optional with wrapped parseValue as default.
It's planned to be released in 14.4.0 and hopefully, will solve current confusion.

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

No branches or pull requests

10 participants