Skip to content

Compatibility with graphql ^15.0.0 #30

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
May 25, 2020
Merged

Compatibility with graphql ^15.0.0 #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dff4047
Switch to new ValidationContext API, use `onError`, remove `getErrors`
robhogan May 23, 2020
38aafdc
Bump graphql dev dependency to ^15.0.0. Bump peer dependency to ^14.5…
robhogan May 23, 2020
5444e27
Replace fieldConfigEstimator usage in tests with fieldExtensionsEstim…
robhogan May 23, 2020
ed7cf16
Remove fieldConfigEstimator and legacyEstimator from README
robhogan May 23, 2020
a1e3b7b
Dev graphql dependency => same as peer dependency
robhogan May 23, 2020
48a6a6c
Make QueryComplexityOptions.estimators mandatory, remove default
robhogan May 23, 2020
cf5fc12
Merge remote-tracking branch 'slicknode/master' into feature/graphql-…
robhogan May 24, 2020
90d39f4
Add graphql@~15.0 to CI matrix
robhogan May 24, 2020
29df455
Add CompatibleValidationContext for use in tests to continue to suppo…
robhogan May 25, 2020
bffa43f
Restore peerDepedencies compatibility with graphql <14.5.0
robhogan May 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ workflows:
# definitions for field extensions in older @types/graphql versions
matrix:
parameters:
graphql-version: ["~0.13", "~14.0", "~14.5", "~14.6"]
graphql-version: ["~0.13", "~14.0", "~14.5", "~14.6", "~15.0"]
- test-and-build:
# Leave graphql-version unspecified to respect the lockfile and also run tsc
name: test-and-build-with-typecheck
Expand Down
4 changes: 0 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,6 @@ or write your own:
schema definition (for example via GraphQL SDL)
* **[`fieldExtensionsEstimator`](src/estimators/fieldExtensions/README.md):** The field extensions estimator lets you set a numeric value or a custom estimator
function in the field config extensions of your schema.
* **[`fieldConfigEstimator`](src/estimators/fieldConfig/README.md):** (DEPRECATED) The field config estimator lets you set a numeric value or a custom estimator
function in the field config of your schema.
* **[`legacyEstimator`](src/estimators/legacy/README.md):** (DEPRECATED) The legacy estimator implements the logic of previous versions. Can be used
to gradually migrate your codebase to new estimators.
* PRs welcome...

Consult the documentation of each estimator for information about how to use them.
Expand Down
5 changes: 2 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"lodash.get": "^4.4.2"
},
"peerDependencies": {
"graphql": "^0.13.0 || ^14.0.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to drop support for 0.13 and 14.0?

Copy link
Contributor Author

@robhogan robhogan May 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of... The issue here is that onError was only added to the constructor args of ValidationContext in 14.5.0.

We don't strictly need that argument outside of tests, and we only pass a dummy function through anyway (since it's now mandatory), so theoretically we still have runtime compatibility with ^0.13.0 || ^14.0.0 because the extra argument we pass will simply be ignored. But, there'd be a typescript error trying to build against those versions, and the tests wouldn't work.

Copy link
Contributor Author

@robhogan robhogan May 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for merging the CI PR - I've just pulled that into this branch to demonstrate the problem here - as you can see the 0.13 and 14.0 tests fail, but this is only because the tests themselves now rely on the new onError API instead of the previous getErrors.

It would be possible to fix this by adding a compatibility shim for use within tests only, which will work with any version of graphql since 0.13:

class CompatibleValidationContext extends ValidationContext {
  private errors: GraphQLError[] = []

  constructor(schema: GraphQLSchema, ast: DocumentNode, typeInfo: TypeInfo) {
    super(schema, ast, typeInfo, err => this.errors.push(err));
  }

  getErrors(): ReadonlyArray<GraphQLError> {
    // @ts-ignore
    return super.getErrors ? super.getErrors() : this.errors
  }
}

So I see three options:

  1. Add (something like) the compatibility layer above for use in tests, and maintain full CI and peer support for ^0.13.0 || ^14.0.0 || ^15.0.0 at the cost of carrying a bit of legacy.
  2. Remove versions below 14.5 from CI, but keep them in peer dependencies. It'll work for now, and keeps tests tidy, but you'd have to keep an eye out for regressions.
  3. Bump peer dependency up to >= 14.5

Your call sir!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed info here! I'd say let's add this compatibility layer (just for the tests) for now with a little note to remove this once we drop support for older GraphQL versions. That should give people time to upgrade their codebases and once we drop support for the old versions, we can remove that temporary compatibility layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, done. I wasn't sure what to call CompatibleValidationLayer or exactly where to put it - let me know if you'd like any of that changing.

Otherwise, all tests now green, and I think we're done?

"graphql": "^0.13.0 || ^14.0.0 || ^15.0.0"
},
"files": [
"dist",
Expand All @@ -41,14 +41,13 @@
"devDependencies": {
"@types/assert": "^1.4.6",
"@types/chai": "^4.2.11",
"@types/graphql": "^0.13.0 || ^14.0.0",
"@types/lodash.get": "^4.4.6",
"@types/mocha": "^7.0.2",
"@typescript-eslint/eslint-plugin": "^2.27.0",
"@typescript-eslint/parser": "^2.27.0",
"chai": "^4.2.0",
"eslint": "^6.8.0",
"graphql": "^0.13.0 || ^14.0.0",
"graphql": "^14.5.0 || ^15.0.0",
"mocha": "^7.1.1",
"rimraf": "^3.0.2",
"ts-node": "^8.8.2",
Expand Down
22 changes: 4 additions & 18 deletions src/QueryComplexity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,6 @@ import {
getNamedType,
GraphQLError
} from 'graphql';
import {
simpleEstimator,
legacyEstimator
} from './estimators';

declare module 'graphql/type/definition' {
export interface GraphQLField<TSource, TContext, TArgs = { [argName: string]: any }> {
Expand Down Expand Up @@ -72,7 +68,7 @@ export interface QueryComplexityOptions {
createError?: (max: number, actual: number) => GraphQLError,

// An array of complexity estimators to use for estimating the complexity
estimators?: Array<ComplexityEstimator>;
estimators: Array<ComplexityEstimator>;
}

function queryComplexityMessage(max: number, actual: number): string {
Expand All @@ -91,7 +87,7 @@ export function getComplexity(options: {
}): number {
const typeInfo = new TypeInfo(options.schema);

const context = new ValidationContext(options.schema, options.query, typeInfo);
const context = new ValidationContext(options.schema, options.query, typeInfo, () => null);
const visitor = new QueryComplexity(context, {
// Maximum complexity does not matter since we're only interested in the calculated complexity.
maximumComplexity: Infinity,
Expand Down Expand Up @@ -127,17 +123,7 @@ export default class QueryComplexity {

this.includeDirectiveDef = this.context.getSchema().getDirective('include');
this.skipDirectiveDef = this.context.getSchema().getDirective('skip');

if (!options.estimators) {
console.warn(
'DEPRECATION WARNING: Estimators should be configured in the queryComplexity options.'
);
}

this.estimators = options.estimators || [
legacyEstimator(),
simpleEstimator()
];
this.estimators = options.estimators

this.OperationDefinition = {
enter: this.onOperationDefinitionEnter,
Expand Down Expand Up @@ -178,7 +164,7 @@ export default class QueryComplexity {
if (typeof this.options.operationName === 'string' && this.options.operationName !== operation.name.value) {
return;
}

if (this.options.onComplete) {
this.options.onComplete(this.complexity);
}
Expand Down
50 changes: 25 additions & 25 deletions src/__tests__/QueryComplexity-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import {
parse,
TypeInfo,
ValidationContext,
visit,
visitWithTypeInfo,
} from 'graphql';
Expand All @@ -18,8 +17,9 @@ import ComplexityVisitor, {getComplexity} from '../QueryComplexity';
import {
simpleEstimator,
directiveEstimator,
fieldConfigEstimator,
fieldExtensionsEstimator,
} from '../index';
import { CompatibleValidationContext } from './fixtures/CompatibleValidationContext';

describe('QueryComplexity analysis', () => {
const typeInfo = new TypeInfo(schema);
Expand Down Expand Up @@ -135,7 +135,7 @@ describe('QueryComplexity analysis', () => {

const complexity = getComplexity({
estimators: [
fieldConfigEstimator(),
fieldExtensionsEstimator(),
simpleEstimator({defaultComplexity: 1})
],
schema,
Expand All @@ -154,7 +154,7 @@ describe('QueryComplexity analysis', () => {
}
`);

const context = new ValidationContext(schema, ast, typeInfo);
const context = new CompatibleValidationContext(schema, ast, typeInfo);
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: [
Expand All @@ -173,11 +173,11 @@ describe('QueryComplexity analysis', () => {
}
`);

const context = new ValidationContext(schema, ast, typeInfo);
const context = new CompatibleValidationContext(schema, ast, typeInfo);
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: [
fieldConfigEstimator(),
fieldExtensionsEstimator(),
simpleEstimator({
defaultComplexity: 1
})
Expand All @@ -203,11 +203,11 @@ describe('QueryComplexity analysis', () => {
}
`);

const context = new ValidationContext(schema, ast, typeInfo);
const context = new CompatibleValidationContext(schema, ast, typeInfo);
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: [
fieldConfigEstimator(),
fieldExtensionsEstimator(),
simpleEstimator({
defaultComplexity: 1
})
Expand All @@ -230,11 +230,11 @@ describe('QueryComplexity analysis', () => {
}
`);

const context = new ValidationContext(schema, ast, typeInfo);
const context = new CompatibleValidationContext(schema, ast, typeInfo);
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: [
fieldConfigEstimator(),
fieldExtensionsEstimator(),
simpleEstimator({
defaultComplexity: 1
})
Expand All @@ -257,11 +257,11 @@ describe('QueryComplexity analysis', () => {
}
`);

const context = new ValidationContext(schema, ast, typeInfo);
const context = new CompatibleValidationContext(schema, ast, typeInfo);
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: [
fieldConfigEstimator(),
fieldExtensionsEstimator(),
simpleEstimator({
defaultComplexity: 1
})
Expand All @@ -284,11 +284,11 @@ describe('QueryComplexity analysis', () => {
}
`);

const context = new ValidationContext(schema, ast, typeInfo);
const context = new CompatibleValidationContext(schema, ast, typeInfo);
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: [
fieldConfigEstimator(),
fieldExtensionsEstimator(),
simpleEstimator({
defaultComplexity: 1
})
Expand All @@ -310,11 +310,11 @@ describe('QueryComplexity analysis', () => {
}
`);

const context = new ValidationContext(schema, ast, typeInfo);
const context = new CompatibleValidationContext(schema, ast, typeInfo);
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: [
fieldConfigEstimator(),
fieldExtensionsEstimator(),
simpleEstimator({
defaultComplexity: 1
})
Expand All @@ -332,11 +332,11 @@ describe('QueryComplexity analysis', () => {
}
`);

const context = new ValidationContext(schema, ast, typeInfo);
const context = new CompatibleValidationContext(schema, ast, typeInfo);
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: [
fieldConfigEstimator(),
fieldExtensionsEstimator(),
simpleEstimator({
defaultComplexity: 1
})
Expand All @@ -353,11 +353,11 @@ describe('QueryComplexity analysis', () => {
requiredArgs
}
`);
const context = new ValidationContext(schema, ast, typeInfo);
const context = new CompatibleValidationContext(schema, ast, typeInfo);
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: [
fieldConfigEstimator(),
fieldExtensionsEstimator(),
simpleEstimator({
defaultComplexity: 1
})
Expand All @@ -374,7 +374,7 @@ describe('QueryComplexity analysis', () => {
scalar
}
`);
const context = new ValidationContext(schema, ast, typeInfo);
const context = new CompatibleValidationContext(schema, ast, typeInfo);
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: []
Expand All @@ -393,11 +393,11 @@ describe('QueryComplexity analysis', () => {
scalar
}
`);
const context = new ValidationContext(schema, ast, typeInfo);
const context = new CompatibleValidationContext(schema, ast, typeInfo);
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: [
fieldConfigEstimator()
fieldExtensionsEstimator()
]
});
visit(ast, visitWithTypeInfo(typeInfo, visitor));
Expand Down Expand Up @@ -463,7 +463,7 @@ describe('QueryComplexity analysis', () => {

const complexity1 = getComplexity({
estimators: [
fieldConfigEstimator(),
fieldExtensionsEstimator(),
simpleEstimator({defaultComplexity: 1})
],
schema,
Expand All @@ -473,7 +473,7 @@ describe('QueryComplexity analysis', () => {

const complexity2 = getComplexity({
estimators: [
fieldConfigEstimator(),
fieldExtensionsEstimator(),
simpleEstimator({defaultComplexity: 1})
],
schema,
Expand Down
28 changes: 28 additions & 0 deletions src/__tests__/fixtures/CompatibleValidationContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { GraphQLError, TypeInfo, ValidationContext } from "graphql";
import { GraphQLSchema } from "graphql/type/schema";
import { DocumentNode } from "graphql/language/ast";

/**
* This class is used to test that validation errors are raised correctly
*
* A compatibility layer is necessary to support graphql versions since 15.0.0
* *as well as* versions prior to 14.5.0 with the same test, because older
* versions of `ValidationContext` only expose a `getErrors` API and newer
* versions only expose the `onError` API via a fourth constructor argument.
*
* Once we drop support for versions older than 14.5.0 this layer will no
* longer be necessary and tests may use `ValidationContext` directly using the
* `onError` API.
*/
export class CompatibleValidationContext extends ValidationContext {
private errors: GraphQLError[] = []

constructor(schema: GraphQLSchema, ast: DocumentNode, typeInfo: TypeInfo) {
super(schema, ast, typeInfo, err => this.errors.push(err));
}

getErrors(): ReadonlyArray<GraphQLError> {
// @ts-ignore
return super.getErrors ? super.getErrors() : this.errors
}
}
20 changes: 14 additions & 6 deletions src/__tests__/fixtures/schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,22 @@ const Item: GraphQLObjectType = new GraphQLObjectType({
fields: () => ({
variableList: {
type: Item,
complexity: (args: ComplexityEstimatorArgs) => args.childComplexity * (args.args.count || 10),
extensions: {
complexity: (args: ComplexityEstimatorArgs) => args.childComplexity * (args.args.count || 10)
},
args: {
count: {
type: GraphQLInt
}
}
},
scalar: { type: GraphQLString },
complexScalar: { type: GraphQLString, complexity: 20 },
complexScalar: { type: GraphQLString, extensions: { complexity: 20 } },
variableScalar: {
type: Item,
complexity: (args: ComplexityEstimatorArgs) => 10 * (args.args.count || 10),
extensions: {
complexity: (args: ComplexityEstimatorArgs) => 10 * (args.args.count || 10)
},
args: {
count: {
type: GraphQLInt
Expand Down Expand Up @@ -97,7 +101,9 @@ const Query = new GraphQLObjectType({
name: { type: GraphQLString },
variableList: {
type: Item,
complexity: (args: ComplexityEstimatorArgs) => args.childComplexity * (args.args.count || 10),
extensions: {
complexity: (args: ComplexityEstimatorArgs) => args.childComplexity * (args.args.count || 10)
},
args: {
count: {
type: GraphQLInt
Expand All @@ -107,11 +113,13 @@ const Query = new GraphQLObjectType({
interface: {type: NameInterface},
enum: {type: EnumType},
scalar: { type: GraphQLString },
complexScalar: { type: GraphQLString, complexity: 20 },
complexScalar: { type: GraphQLString, extensions: { complexity: 20 } },
union: { type: Union },
variableScalar: {
type: Item,
complexity: (args: ComplexityEstimatorArgs) => 10 * (args.args.count || 10),
extensions: {
complexity: (args: ComplexityEstimatorArgs) => 10 * (args.args.count || 10)
},
args: {
count: {
type: GraphQLInt
Expand Down
Loading