suggestions based on original review comments #7
Conversation
src/utilities/TypeInfo.ts
Outdated
| @@ -38,6 +39,14 @@ import type { GraphQLSchema } from '../type/schema.js'; | |||
| import { typeFromAST } from './typeFromAST.js'; | |||
| import { valueFromAST } from './valueFromAST.js'; | |||
|
|
|||
| export interface GraphQLSignature { | |||
There was a problem hiding this comment.
Alternatively, we could introduce a new GraphQLFragmentVariableSignature interface. If we wanted we could also have a union type GraphQLSignature = GraphQLArgument | GraphQLFragmentVariableSignature.
I believe the basic learning from this PR is that we can't have a union over VariableDefinitionNode itself, because we have to validate the type and generate the actual default value.
src/utilities/TypeInfo.ts
Outdated
| @@ -129,6 +140,10 @@ export class TypeInfo { | |||
| return this._argument; | |||
| } | |||
|
|
|||
| getSignature(): Maybe<GraphQLSignature> { | |||
There was a problem hiding this comment.
Alternatively we could expose a getFragmentVariableSignature() helper that only returns a value if we are within an argument for a fragment variable. Or we could have both the more specific and the more generic helpers.
|
Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
JoviDeCroock
force-pushed
the
fragment-args-typeinfo-2024
branch
3 times, most recently
from
1190b79 to
1d770fb
Compare
JoviDeCroock
force-pushed
the
fragment-args-typeinfo-2024
branch
2 times, most recently
from
9de28dc to
b422131
Compare
yaacovCR
force-pushed
the
review-feedback
branch
3 times, most recently
from
6c1a90f to
dac365d
Compare
yaacovCR
force-pushed
the
review-feedback
branch
from
dac365d to
b422131
Compare
introduces new signature type that works across GraphQLArgument and VariableDefinitionNode, retaining getArgument() for backwards compatibility for the time being.
the signature type checks to make sure that there is an input type for the fragment variable and generates the default value.
also generates the appropriate fragment variable "signatures" once per document, as suggested, although without the schema pieces