Add 'getDefinitionsMap' to 'ASTValidationContext' #1476
Conversation
| @@ -22,38 +22,25 @@ export function unusedFragMessage(fragName: string): string { | |||
| * within operations, or spread within other fragments spread within operations. | |||
| */ | |||
| export function NoUnusedFragments(context: ValidationContext): ASTVisitor { | |||
| const operationDefs = []; | |||
| const fragmentDefs = []; | |||
There was a problem hiding this comment.
In the context of GQL documents getDefinitionsMap is a just reusable version of this code.
There was a problem hiding this comment.
This is very clever. I like the idea behind it a lot. I'm not sure it makes sense to expose publicly yet, but for the internal rules it makes a lot of sense to use something like getDefinitionsMap directly.
IvanGoncharov
force-pushed
the
SDLContextGetDirective
branch
from
62bcd86 to
f1d0a02
Compare
|
@mjmahone Can you please take a look? |
| @@ -22,38 +22,25 @@ export function unusedFragMessage(fragName: string): string { | |||
| * within operations, or spread within other fragments spread within operations. | |||
| */ | |||
| export function NoUnusedFragments(context: ValidationContext): ASTVisitor { | |||
| const operationDefs = []; | |||
| const fragmentDefs = []; | |||
There was a problem hiding this comment.
This is very clever. I like the idea behind it a lot. I'm not sure it makes sense to expose publicly yet, but for the internal rules it makes a lot of sense to use something like getDefinitionsMap directly.
| ); | ||
| } | ||
| return fragments[name]; | ||
| return this._fragments[name]; |
There was a problem hiding this comment.
We could still move the this._fragments instantiation to here:
if (!this._fragments) {
const fragments = keyMap(...);
this._fragments = fragments;
return fragments[name];
}
return this._fragments[name];
No point incurring the cost of object-creation until someone actually asks for it.
| if (def.kind === Kind.DIRECTIVE_DEFINITION) { | ||
| locationsMap[def.name.value] = def.locations.map(name => name.value); | ||
| } | ||
| const defNodes = context.getDefinitionsMap().DirectiveDefinition || []; |
There was a problem hiding this comment.
I think it makes more sense to have both contexts have a getDirectiveDefinitions() by having ASTValidationContext define the function. On the other hand, if we can hide getDefinitionsMap somehow from being publicly exposed, I'm OK with the current API.
One nice thing with not exposing it: we could later easily add a getFragmentDefinitions() and getOperationDefinitions() using the same private structure, without ever publicly exposing the somewhat complex KindToNodesMap.
My big worry with getDefinitionsMap is that the type signature for it is a bit strange to me. It's very clever, which makes it hard for a newcomer to immediately grasp. Whereas getDirectiveDefinitions(): $ReadOnlyArray<DirectiveDefinitionNode> obviously returns all the known directives, and the type signature supports that.
IvanGoncharov
force-pushed
the
SDLContextGetDirective
branch
from
f1d0a02 to
3490960
Compare
@mjmahone I finally figure out how to address your comment:
#1463 (comment)
Can you please review this PR so I could rebase #1463 on top of it?