graphql-js icon indicating copy to clipboard operation
graphql-js copied to clipboard

Make variableDefinitions not nullable

Open brandon-leapyear opened this issue 5 years ago • 1 comments

:sparkles: This is an old work account. Please reference @brandonchinn178 for all future communication :sparkles:


It seems like variableDefinitions will always be a non-null array.

https://github.com/graphql/graphql-js/blob/f07ec241d51d7c3fb0263bb3ade1975dc87babc3/src/language/parser.js#L258-L285 https://github.com/graphql/graphql-js/blob/f07ec241d51d7c3fb0263bb3ade1975dc87babc3/src/language/parser.js#L307

brandon-leapyear avatar Jan 27 '20 23:01 brandon-leapyear

Hi @brandon-leapyear

Thanks for PR 👍 AST format should have a healthy balance between the ability to easily process but at the same to easily construct AST nodes. For example, at the moment you can easily extract part of the query into separate query:

return {
  kind: Kind.OPERATION_DEFINITION, 
  selectionSet: someSelectionSet,
};

And you don't need to add a bunch of empty arrays, moreover, it makes code less fragile when we add new properties to AST nodes.

But I agree it looks weird and creates complications, for example with testing see #2203 Personally I think a good long term solution would be to always return undefined instead of empty arrays since it's will improve performance and memory consumption. Also, it would be great if someone researches what other JS parsers do e.g. Babel's parser, ESLint parser, etc.

IvanGoncharov avatar Jan 28 '20 04:01 IvanGoncharov