Skip to content

Commit

Permalink
Remove 'devAssert' checks that duplicate TS types (#3642)
Browse files Browse the repository at this point in the history
`graphql` provides TS types since `14.5.0` (released 3 years ago)
and we fully switched to TS in `15.0.0` so I think it's time to drop
runtime typechecks.

Motivations: This type checks were added long time ago since we shifted
towards TS we just maintained them without adding new ones.
In general, this check increase bundle size add runtime cost and we
can't realistically check all arguments to all functions.
Instead we should focus on adding more asserts on stuff that can't be
checked by TS.
  • Loading branch information
IvanGoncharov authored Jun 13, 2022
1 parent e414279 commit a4b085b
Show file tree
Hide file tree
Showing 21 changed files with 45 additions and 664 deletions.
48 changes: 0 additions & 48 deletions src/execution/__tests__/executor-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,54 +22,6 @@ import { GraphQLSchema } from '../../type/schema';
import { execute, executeSync } from '../execute';

describe('Execute: Handles basic execution tasks', () => {
it('throws if no document is provided', () => {
const schema = new GraphQLSchema({
query: new GraphQLObjectType({
name: 'Type',
fields: {
a: { type: GraphQLString },
},
}),
});

// @ts-expect-error
expect(() => executeSync({ schema })).to.throw('Must provide document.');
});

it('throws if no schema is provided', () => {
const document = parse('{ field }');

// @ts-expect-error
expect(() => executeSync({ document })).to.throw(
'Expected undefined to be a GraphQL schema.',
);
});

it('throws on invalid variables', () => {
const schema = new GraphQLSchema({
query: new GraphQLObjectType({
name: 'Type',
fields: {
fieldA: {
type: GraphQLString,
args: { argA: { type: GraphQLInt } },
},
},
}),
});
const document = parse(`
query ($a: Int) {
fieldA(argA: $a)
}
`);
const variableValues = '{ "a": 1 }';

// @ts-expect-error
expect(() => executeSync({ schema, document, variableValues })).to.throw(
'Variables must be provided as an Object where each property is a variable value. Perhaps look to see if an unparsed JSON string was provided.',
);
});

it('executes arbitrary code', async () => {
const data = {
a: () => 'Apple',
Expand Down
31 changes: 0 additions & 31 deletions src/execution/__tests__/subscribe-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -390,37 +390,6 @@ describe('Subscription Initialization Phase', () => {
});
});

it('throws an error if some of required arguments are missing', async () => {
const document = parse('subscription { foo }');
const schema = new GraphQLSchema({
query: DummyQueryType,
subscription: new GraphQLObjectType({
name: 'Subscription',
fields: {
foo: { type: GraphQLString },
},
}),
});

// @ts-expect-error (schema must not be null)
expect(() => subscribe({ schema: null, document })).to.throw(
'Expected null to be a GraphQL schema.',
);

// @ts-expect-error
expect(() => subscribe({ document })).to.throw(
'Expected undefined to be a GraphQL schema.',
);

// @ts-expect-error (document must not be null)
expect(() => subscribe({ schema, document: null })).to.throw(
'Must provide document.',
);

// @ts-expect-error
expect(() => subscribe({ schema })).to.throw('Must provide document.');
});

it('resolves to an error if schema does not support subscriptions', async () => {
const schema = new GraphQLSchema({ query: DummyQueryType });
const document = parse('subscription { unknownField }');
Expand Down
17 changes: 2 additions & 15 deletions src/execution/execute.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { devAssert } from '../jsutils/devAssert';
import { inspect } from '../jsutils/inspect';
import { invariant } from '../jsutils/invariant';
import { isAsyncIterable } from '../jsutils/isAsyncIterable';
Expand Down Expand Up @@ -239,21 +238,9 @@ function buildResponse(
* TODO: consider no longer exporting this function
* @internal
*/
export function assertValidExecutionArguments(
schema: GraphQLSchema,
document: DocumentNode,
rawVariableValues: Maybe<{ readonly [variable: string]: unknown }>,
): void {
devAssert(document != null, 'Must provide document.');

export function assertValidExecutionArguments(schema: GraphQLSchema): void {
// If the schema used for execution is invalid, throw an error.
assertValidSchema(schema);

// Variables, if provided, must be an object.
devAssert(
rawVariableValues == null || isObjectLike(rawVariableValues),
'Variables must be provided as an Object where each property is a variable value. Perhaps look to see if an unparsed JSON string was provided.',
);
}

/**
Expand Down Expand Up @@ -281,7 +268,7 @@ export function buildExecutionContext(
} = args;

// If arguments are missing or incorrect, throw an error.
assertValidExecutionArguments(schema, document, rawVariableValues);
assertValidExecutionArguments(schema);

let operation: OperationDefinitionNode | undefined;
const fragments: ObjMap<FragmentDefinitionNode> = Object.create(null);
Expand Down
14 changes: 0 additions & 14 deletions src/language/__tests__/source-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,6 @@ import { describe, it } from 'mocha';
import { Source } from '../source';

describe('Source', () => {
it('asserts that a body was provided', () => {
// @ts-expect-error
expect(() => new Source()).to.throw(
'Body must be a string. Received: undefined.',
);
});

it('asserts that a valid body was provided', () => {
// @ts-expect-error
expect(() => new Source({})).to.throw(
'Body must be a string. Received: {}.',
);
});

it('can be Object.toStringified', () => {
const source = new Source('');

Expand Down
6 changes: 0 additions & 6 deletions src/language/source.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { devAssert } from '../jsutils/devAssert';
import { inspect } from '../jsutils/inspect';
import { instanceOf } from '../jsutils/instanceOf';

interface Location {
Expand All @@ -24,11 +23,6 @@ export class Source {
name: string = 'GraphQL request',
locationOffset: Location = { line: 1, column: 1 },
) {
devAssert(
typeof body === 'string',
`Body must be a string. Received: ${inspect(body)}.`,
);

this.body = body;
this.name = name;
this.locationOffset = locationOffset;
Expand Down
5 changes: 0 additions & 5 deletions src/type/__tests__/assertName-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,6 @@ describe('assertName', () => {
expect(assertName('_ValidName123')).to.equal('_ValidName123');
});

it('throws for non-strings', () => {
// @ts-expect-error
expect(() => assertName({})).to.throw('Expected name to be a string.');
});

it('throws on empty strings', () => {
expect(() => assertName('')).to.throw(
'Expected name to be a non-empty string.',
Expand Down
Loading

0 comments on commit a4b085b

Please sign in to comment.