From d6d17ac145c32893bdd65d53a46d38122b47ca5e Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Wed, 26 May 2021 14:45:47 -0700 Subject: [PATCH] TS: Fix strict issues in src/validation/ * Add `"strict": true` to tsconfig.json * Fix all issues that arise within `src/validation` --- src/type/definition.ts | 6 ++ src/validation/ValidationContext.ts | 22 ++++--- src/validation/__tests__/validation-test.ts | 4 +- src/validation/rules/KnownDirectivesRule.ts | 6 +- src/validation/rules/KnownTypeNamesRule.ts | 3 +- src/validation/rules/NoFragmentCyclesRule.ts | 13 ++-- src/validation/rules/NoUnusedFragmentsRule.ts | 8 ++- src/validation/rules/NoUnusedVariablesRule.ts | 3 +- .../rules/OverlappingFieldsCanBeMergedRule.ts | 62 ++++++++++--------- .../rules/PossibleTypeExtensionsRule.ts | 9 +-- .../rules/UniqueDirectivesPerLocationRule.ts | 4 +- .../rules/UniqueInputFieldNamesRule.ts | 12 +++- src/validation/validate.ts | 4 +- 13 files changed, 91 insertions(+), 65 deletions(-) diff --git a/src/type/definition.ts b/src/type/definition.ts index 3d38a43d5a2..f7a46c31eab 100644 --- a/src/type/definition.ts +++ b/src/type/definition.ts @@ -457,6 +457,9 @@ export function getNullableType(type: undefined | null): void; export function getNullableType( type: T | GraphQLNonNull, ): T; +export function getNullableType( + type: Maybe, +): GraphQLNullableType | undefined; export function getNullableType( type: Maybe, ): GraphQLNullableType | undefined { @@ -504,6 +507,9 @@ export function getNamedType(type: undefined | null): void; export function getNamedType(type: GraphQLInputType): GraphQLNamedInputType; export function getNamedType(type: GraphQLOutputType): GraphQLNamedOutputType; export function getNamedType(type: GraphQLType): GraphQLNamedType; +export function getNamedType( + type: Maybe, +): GraphQLNamedType | undefined; export function getNamedType( type: Maybe, ): GraphQLNamedType | undefined { diff --git a/src/validation/ValidationContext.ts b/src/validation/ValidationContext.ts index 0479318b2a2..ba372adccdb 100644 --- a/src/validation/ValidationContext.ts +++ b/src/validation/ValidationContext.ts @@ -44,7 +44,7 @@ interface VariableUsage { export class ASTValidationContext { private _ast: DocumentNode; private _onError: (error: GraphQLError) => void; - private _fragments: Maybe>; + private _fragments: ObjMap | undefined; private _fragmentSpreads: Map< SelectionSetNode, ReadonlyArray @@ -72,15 +72,19 @@ export class ASTValidationContext { } getFragment(name: string): Maybe { - if (!this._fragments) { - const fragments = (this._fragments = Object.create(null)); + let fragments: ObjMap; + if (this._fragments) { + fragments = this._fragments; + } else { + fragments = Object.create(null); for (const defNode of this.getDocument().definitions) { if (defNode.kind === Kind.FRAGMENT_DEFINITION) { fragments[defNode.name.value] = defNode; } } + this._fragments = fragments; } - return this._fragments[name]; + return fragments[name]; } getFragmentSpreads( @@ -90,8 +94,8 @@ export class ASTValidationContext { if (!spreads) { spreads = []; const setsToVisit: Array = [node]; - while (setsToVisit.length !== 0) { - const set = setsToVisit.pop(); + let set: SelectionSetNode | undefined; + while ((set = setsToVisit.pop())) { for (const selection of set.selections) { if (selection.kind === Kind.FRAGMENT_SPREAD) { // @ts-expect-error FIXME: TS Conversion @@ -114,8 +118,8 @@ export class ASTValidationContext { fragments = []; const collectedNames = Object.create(null); const nodesToVisit: Array = [operation.selectionSet]; - while (nodesToVisit.length !== 0) { - const node = nodesToVisit.pop(); + let node: SelectionSetNode | undefined; + while ((node = nodesToVisit.pop())) { for (const spread of this.getFragmentSpreads(node)) { const fragName = spread.name.value; if (collectedNames[fragName] !== true) { @@ -189,7 +193,7 @@ export class ValidationContext extends ASTValidationContext { getVariableUsages(node: NodeWithSelectionSet): ReadonlyArray { let usages = this._variableUsages.get(node); if (!usages) { - const newUsages = []; + const newUsages: Array = []; const typeInfo = new TypeInfo(this._schema); visit( node, diff --git a/src/validation/__tests__/validation-test.ts b/src/validation/__tests__/validation-test.ts index a015bb9bd44..42860051fc4 100644 --- a/src/validation/__tests__/validation-test.ts +++ b/src/validation/__tests__/validation-test.ts @@ -3,6 +3,7 @@ import { describe, it } from 'mocha'; import { GraphQLError } from '../../error/GraphQLError'; +import type { DirectiveNode } from '../../language/ast'; import { parse } from '../../language/parser'; import { TypeInfo } from '../../utilities/TypeInfo'; @@ -15,6 +16,7 @@ import { testSchema } from './harness'; describe('Validate: Supports full validation', () => { it('rejects invalid documents', () => { + // TODO ts-expect-error (expects a DocumentNode as a second parameter) expect(() => validate(testSchema, null)).to.throw('Must provide document.'); }); @@ -96,7 +98,7 @@ describe('Validate: Supports full validation', () => { function customRule(context: ValidationContext) { return { - Directive(node) { + Directive(node: DirectiveNode) { const directiveDef = context.getDirective(); const error = new GraphQLError( 'Reporting directive: ' + String(directiveDef), diff --git a/src/validation/rules/KnownDirectivesRule.ts b/src/validation/rules/KnownDirectivesRule.ts index 7418e570e1d..652bdde186d 100644 --- a/src/validation/rules/KnownDirectivesRule.ts +++ b/src/validation/rules/KnownDirectivesRule.ts @@ -71,12 +71,10 @@ function getDirectiveLocationForASTPath( ancestors: ReadonlyArray>, ): DirectiveLocationEnum | undefined { const appliedTo = ancestors[ancestors.length - 1]; - invariant(!Array.isArray(appliedTo)); + invariant('kind' in appliedTo); - // @ts-expect-error FIXME: TS Conversion switch (appliedTo.kind) { case Kind.OPERATION_DEFINITION: - // @ts-expect-error FIXME: TS Conversion return getDirectiveLocationForOperation(appliedTo.operation); case Kind.FIELD: return DirectiveLocation.FIELD; @@ -115,7 +113,7 @@ function getDirectiveLocationForASTPath( return DirectiveLocation.INPUT_OBJECT; case Kind.INPUT_VALUE_DEFINITION: { const parentNode = ancestors[ancestors.length - 3]; - // @ts-expect-error FIXME: TS Conversion + invariant('kind' in parentNode); return parentNode.kind === Kind.INPUT_OBJECT_TYPE_DEFINITION ? DirectiveLocation.INPUT_FIELD_DEFINITION : DirectiveLocation.ARGUMENT_DEFINITION; diff --git a/src/validation/rules/KnownTypeNamesRule.ts b/src/validation/rules/KnownTypeNamesRule.ts index 8593e9a5a24..61d8981c3b7 100644 --- a/src/validation/rules/KnownTypeNamesRule.ts +++ b/src/validation/rules/KnownTypeNamesRule.ts @@ -74,8 +74,7 @@ const standardTypeNames = [...specifiedScalarTypes, ...introspectionTypes].map( function isSDLNode(value: ASTNode | ReadonlyArray): boolean { return ( - !Array.isArray(value) && - // @ts-expect-error FIXME: TS Conversion + 'kind' in value && (isTypeSystemDefinitionNode(value) || isTypeSystemExtensionNode(value)) ); } diff --git a/src/validation/rules/NoFragmentCyclesRule.ts b/src/validation/rules/NoFragmentCyclesRule.ts index 54fba26e438..eb913917315 100644 --- a/src/validation/rules/NoFragmentCyclesRule.ts +++ b/src/validation/rules/NoFragmentCyclesRule.ts @@ -1,7 +1,12 @@ +import type { ObjMap } from '../../jsutils/ObjMap'; + import { GraphQLError } from '../../error/GraphQLError'; +import type { + FragmentDefinitionNode, + FragmentSpreadNode, +} from '../../language/ast'; import type { ASTVisitor } from '../../language/visitor'; -import type { FragmentDefinitionNode } from '../../language/ast'; import type { ASTValidationContext } from '../ValidationContext'; @@ -10,13 +15,13 @@ export function NoFragmentCyclesRule( ): ASTVisitor { // Tracks already visited fragments to maintain O(N) and to ensure that cycles // are not redundantly reported. - const visitedFrags = Object.create(null); + const visitedFrags: ObjMap = Object.create(null); // Array of AST nodes used to produce meaningful errors - const spreadPath = []; + const spreadPath: Array = []; // Position in the spread path - const spreadPathIndexByName = Object.create(null); + const spreadPathIndexByName: ObjMap = Object.create(null); return { OperationDefinition: () => false, diff --git a/src/validation/rules/NoUnusedFragmentsRule.ts b/src/validation/rules/NoUnusedFragmentsRule.ts index d69bf241cfa..393f783d728 100644 --- a/src/validation/rules/NoUnusedFragmentsRule.ts +++ b/src/validation/rules/NoUnusedFragmentsRule.ts @@ -1,5 +1,9 @@ import { GraphQLError } from '../../error/GraphQLError'; +import type { + FragmentDefinitionNode, + OperationDefinitionNode, +} from '../../language/ast'; import type { ASTVisitor } from '../../language/visitor'; import type { ASTValidationContext } from '../ValidationContext'; @@ -13,8 +17,8 @@ import type { ASTValidationContext } from '../ValidationContext'; export function NoUnusedFragmentsRule( context: ASTValidationContext, ): ASTVisitor { - const operationDefs = []; - const fragmentDefs = []; + const operationDefs: Array = []; + const fragmentDefs: Array = []; return { OperationDefinition(node) { diff --git a/src/validation/rules/NoUnusedVariablesRule.ts b/src/validation/rules/NoUnusedVariablesRule.ts index 70bc81c9419..c58c390f0bc 100644 --- a/src/validation/rules/NoUnusedVariablesRule.ts +++ b/src/validation/rules/NoUnusedVariablesRule.ts @@ -1,5 +1,6 @@ import { GraphQLError } from '../../error/GraphQLError'; +import type { VariableDefinitionNode } from '../../language/ast'; import type { ASTVisitor } from '../../language/visitor'; import type { ValidationContext } from '../ValidationContext'; @@ -11,7 +12,7 @@ import type { ValidationContext } from '../ValidationContext'; * are used, either directly or within a spread fragment. */ export function NoUnusedVariablesRule(context: ValidationContext): ASTVisitor { - let variableDefs = []; + let variableDefs: Array = []; return { OperationDefinition: { diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 2ae5468cbff..28037485d3e 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -17,7 +17,6 @@ import { print } from '../../language/printer'; import type { GraphQLNamedType, GraphQLOutputType, - GraphQLCompositeType, GraphQLField, } from '../../type/definition'; import { @@ -97,12 +96,14 @@ type ConflictReason = [string, ConflictReasonMessage]; type ConflictReasonMessage = string | Array; // Tuple defining a field node in a context. type NodeAndDef = [ - GraphQLCompositeType, + Maybe, FieldNode, Maybe>, ]; // Map of array of those. type NodeAndDefCollection = ObjMap>; +type FragmentNames = Array; +type FieldsAndFragmentNames = readonly [NodeAndDefCollection, FragmentNames]; /** * Algorithm: @@ -164,12 +165,12 @@ type NodeAndDefCollection = ObjMap>; // GraphQL Document. function findConflictsWithinSelectionSet( context: ValidationContext, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentNames: Map, comparedFragmentPairs: PairSet, parentType: Maybe, selectionSet: SelectionSetNode, ): Array { - const conflicts = []; + const conflicts: Array = []; const [fieldMap, fragmentNames] = getFieldsAndFragmentNames( context, @@ -226,7 +227,7 @@ function findConflictsWithinSelectionSet( function collectConflictsBetweenFieldsAndFragment( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentNames: Map, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fieldMap: NodeAndDefCollection, @@ -280,7 +281,7 @@ function collectConflictsBetweenFieldsAndFragment( function collectConflictsBetweenFragments( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentNames: Map, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fragmentName1: string, @@ -366,7 +367,7 @@ function collectConflictsBetweenFragments( // between the sub-fields of two overlapping fields. function findConflictsBetweenSubSelectionSets( context: ValidationContext, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentNames: Map, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, parentType1: Maybe, @@ -374,7 +375,7 @@ function findConflictsBetweenSubSelectionSets( parentType2: Maybe, selectionSet2: SelectionSetNode, ): Array { - const conflicts = []; + const conflicts: Array = []; const [fieldMap1, fragmentNames1] = getFieldsAndFragmentNames( context, @@ -455,7 +456,7 @@ function findConflictsBetweenSubSelectionSets( function collectConflictsWithin( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentNames: Map, comparedFragmentPairs: PairSet, fieldMap: NodeAndDefCollection, ): void { @@ -496,7 +497,7 @@ function collectConflictsWithin( function collectConflictsBetween( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentNames: Map, comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, fieldMap1: NodeAndDefCollection, @@ -534,7 +535,7 @@ function collectConflictsBetween( // comparing their sub-fields. function findConflict( context: ValidationContext, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentNames: Map, comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, responseName: string, @@ -677,32 +678,33 @@ function doTypesConflict( // referenced via fragment spreads. function getFieldsAndFragmentNames( context: ValidationContext, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentNames: Map, parentType: Maybe, selectionSet: SelectionSetNode, -): [NodeAndDefCollection, Array] { - let cached = cachedFieldsAndFragmentNames.get(selectionSet); - if (!cached) { - const nodeAndDefs = Object.create(null); - const fragmentNames = Object.create(null); - _collectFieldsAndFragmentNames( - context, - parentType, - selectionSet, - nodeAndDefs, - fragmentNames, - ); - cached = [nodeAndDefs, Object.keys(fragmentNames)]; - cachedFieldsAndFragmentNames.set(selectionSet, cached); +): FieldsAndFragmentNames { + const cached = cachedFieldsAndFragmentNames.get(selectionSet); + if (cached) { + return cached; } - return cached; + const nodeAndDefs: NodeAndDefCollection = Object.create(null); + const fragmentNames: ObjMap = Object.create(null); + _collectFieldsAndFragmentNames( + context, + parentType, + selectionSet, + nodeAndDefs, + fragmentNames, + ); + const result = [nodeAndDefs, Object.keys(fragmentNames)] as const; + cachedFieldsAndFragmentNames.set(selectionSet, result); + return result; } // Given a reference to a fragment, return the represented collection of fields // as well as a list of nested fragment names referenced via fragment spreads. function getReferencedFieldsAndFragmentNames( context: ValidationContext, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentNames: Map, fragment: FragmentDefinitionNode, ) { // Short-circuit building a type from the node if possible. @@ -724,8 +726,8 @@ function _collectFieldsAndFragmentNames( context: ValidationContext, parentType: Maybe, selectionSet: SelectionSetNode, - nodeAndDefs, - fragmentNames, + nodeAndDefs: NodeAndDefCollection, + fragmentNames: ObjMap, ): void { for (const selection of selectionSet.selections) { switch (selection.kind) { diff --git a/src/validation/rules/PossibleTypeExtensionsRule.ts b/src/validation/rules/PossibleTypeExtensionsRule.ts index ab7db3105b6..11c55eff841 100644 --- a/src/validation/rules/PossibleTypeExtensionsRule.ts +++ b/src/validation/rules/PossibleTypeExtensionsRule.ts @@ -1,3 +1,4 @@ +import type { ObjMap } from '../../jsutils/ObjMap'; import { inspect } from '../../jsutils/inspect'; import { invariant } from '../../jsutils/invariant'; import { didYouMean } from '../../jsutils/didYouMean'; @@ -7,7 +8,7 @@ import { GraphQLError } from '../../error/GraphQLError'; import type { KindEnum } from '../../language/kinds'; import type { ASTVisitor } from '../../language/visitor'; -import type { TypeExtensionNode } from '../../language/ast'; +import type { DefinitionNode, TypeExtensionNode } from '../../language/ast'; import { Kind } from '../../language/kinds'; import { isTypeDefinitionNode } from '../../language/predicates'; @@ -32,7 +33,7 @@ export function PossibleTypeExtensionsRule( context: SDLValidationContext, ): ASTVisitor { const schema = context.getSchema(); - const definedTypes = Object.create(null); + const definedTypes: ObjMap = Object.create(null); for (const def of context.getDocument().definitions) { if (isTypeDefinitionNode(def)) { @@ -54,7 +55,7 @@ export function PossibleTypeExtensionsRule( const defNode = definedTypes[typeName]; const existingType = schema?.getType(typeName); - let expectedKind; + let expectedKind: KindEnum | undefined; if (defNode) { expectedKind = defKindToExtKind[defNode.kind]; } else if (existingType) { @@ -89,7 +90,7 @@ export function PossibleTypeExtensionsRule( } } -const defKindToExtKind = { +const defKindToExtKind: ObjMap = { [Kind.SCALAR_TYPE_DEFINITION]: Kind.SCALAR_TYPE_EXTENSION, [Kind.OBJECT_TYPE_DEFINITION]: Kind.OBJECT_TYPE_EXTENSION, [Kind.INTERFACE_TYPE_DEFINITION]: Kind.INTERFACE_TYPE_EXTENSION, diff --git a/src/validation/rules/UniqueDirectivesPerLocationRule.ts b/src/validation/rules/UniqueDirectivesPerLocationRule.ts index 8bdff5de240..85bea14969b 100644 --- a/src/validation/rules/UniqueDirectivesPerLocationRule.ts +++ b/src/validation/rules/UniqueDirectivesPerLocationRule.ts @@ -48,8 +48,7 @@ export function UniqueDirectivesPerLocationRule( // them all, just listen for entering any node, and check to see if it // defines any directives. enter(node) { - // @ts-expect-error FIXME: TS Conversion - if (node.directives == null) { + if (!('directives' in node) || !node.directives) { return; } @@ -69,7 +68,6 @@ export function UniqueDirectivesPerLocationRule( seenDirectives = Object.create(null); } - // @ts-expect-error FIXME: TS Conversion for (const directive of node.directives) { const directiveName = directive.name.value; diff --git a/src/validation/rules/UniqueInputFieldNamesRule.ts b/src/validation/rules/UniqueInputFieldNamesRule.ts index 413783e9301..cd6b1161c67 100644 --- a/src/validation/rules/UniqueInputFieldNamesRule.ts +++ b/src/validation/rules/UniqueInputFieldNamesRule.ts @@ -1,5 +1,9 @@ +import type { ObjMap } from '../../jsutils/ObjMap'; +import { invariant } from '../../jsutils/invariant'; + import { GraphQLError } from '../../error/GraphQLError'; +import type { NameNode } from '../../language/ast'; import type { ASTVisitor } from '../../language/visitor'; import type { ASTValidationContext } from '../ValidationContext'; @@ -13,8 +17,8 @@ import type { ASTValidationContext } from '../ValidationContext'; export function UniqueInputFieldNamesRule( context: ASTValidationContext, ): ASTVisitor { - const knownNameStack = []; - let knownNames = Object.create(null); + const knownNameStack: Array> = []; + let knownNames: ObjMap = Object.create(null); return { ObjectValue: { @@ -23,7 +27,9 @@ export function UniqueInputFieldNamesRule( knownNames = Object.create(null); }, leave() { - knownNames = knownNameStack.pop(); + const prevKnownNames = knownNameStack.pop(); + invariant(prevKnownNames); + knownNames = prevKnownNames; }, }, ObjectField(node) { diff --git a/src/validation/validate.ts b/src/validation/validate.ts index 5988115aaee..511942155f3 100644 --- a/src/validation/validate.ts +++ b/src/validation/validate.ts @@ -45,7 +45,7 @@ export function validate( assertValidSchema(schema); const abortObj = Object.freeze({}); - const errors = []; + const errors: Array = []; const context = new ValidationContext( schema, documentAST, @@ -87,7 +87,7 @@ export function validateSDL( schemaToExtend?: Maybe, rules: ReadonlyArray = specifiedSDLRules, ): ReadonlyArray { - const errors = []; + const errors: Array = []; const context = new SDLValidationContext( documentAST, schemaToExtend,