Should parseLiteral throw a TypeError when provided an invalid value?

[danielrearden]

When calling a GraphQLScalarType's parseLiteral or parseValue methods, those methods can either return undefined or throw an error -- in either case, the value is considered invalid and an error is included in the response. However, returning undefined results in a generic error message about the type being invalid, while throwing a TypeError allows us to provide additional details as to why the provided value was invalid.

For all built-in scalars, parseValue throws a TypeError when the value is invalid, while parseLiteral always returns undefined. This results in inconsistent messaging when validating literal inputs compared to variables. For example:

Expected type Int, found 2147483648.

compared with

Variable "$first" got invalid value 2147483648; Expected type Int; Int cannot represent non 32-bit signed integer value: 2147483648

Anywhere parseLiteral is actually called, it's already wrapped with a try/catch and any error is handled appropriately -- namely here and here.

I know this is fairly trivial, but is there any reason to avoid throwing a TypeError inside parseLiteral when the value is invalid? Even if parseLiteral and parseValue can't share the exact same logic, it might make sense to reconcile them, at least in that regard. Would you accept a PR to that affect?

[IvanGoncharov]

@danielrearden Thanks for bringing this up and especially for deep research 👍

I know this is fairly trivial, but is there any reason to avoid throwing a TypeError inside parseLiteral when the value is invalid?

As I understand, it's like that because initially, it was problematic to parse SDL with complex default values. So returning undefined was introduced as a temporary workaround but the current valueFromAST implementation already solves this problem and makes workaround unnecessary.

Would you accept a PR to that affect?

Yes, it would be great. One note though: parseLiteral is part of public API so technically it would be a breaking change so we can't merge it in 14.x line.
But we plan to start working on 15.0.0 next month and can merge PR at the beginning of this process.

[IvanGoncharov]

This enhancement is tracked in #1827