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 5 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
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": "^14.5.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
8 changes: 3 additions & 5 deletions src/QueryComplexity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ import {
GraphQLError
} from 'graphql';
import {
simpleEstimator,
legacyEstimator
simpleEstimator
} from './estimators';

declare module 'graphql/type/definition' {
Expand Down Expand Up @@ -91,7 +90,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 @@ -135,7 +134,6 @@ export default class QueryComplexity {
}

this.estimators = options.estimators || [
legacyEstimator(),
simpleEstimator()
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ivome this might be a good time to make options.estimators required, since this PR requires changing the default estimators anyway?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea, let's make this required now with all the breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


Expand Down Expand Up @@ -178,7 +176,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
69 changes: 37 additions & 32 deletions src/__tests__/QueryComplexity-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/

import {
GraphQLError,
parse,
TypeInfo,
ValidationContext,
Expand All @@ -18,7 +19,7 @@ import ComplexityVisitor, {getComplexity} from '../QueryComplexity';
import {
simpleEstimator,
directiveEstimator,
fieldConfigEstimator,
fieldExtensionsEstimator,
} from '../index';

describe('QueryComplexity analysis', () => {
Expand Down Expand Up @@ -135,7 +136,7 @@ describe('QueryComplexity analysis', () => {

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

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

const context = new ValidationContext(schema, ast, typeInfo);
const validationErrors: GraphQLError[] = [];
const context = new ValidationContext(schema, ast, typeInfo, err => validationErrors.push(err));
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: [
fieldConfigEstimator(),
fieldExtensionsEstimator(),
simpleEstimator({
defaultComplexity: 1
})
Expand All @@ -186,8 +188,8 @@ describe('QueryComplexity analysis', () => {

visit(ast, visitWithTypeInfo(typeInfo, visitor));
expect(visitor.complexity).to.equal(1000);
expect(context.getErrors().length).to.equal(1);
expect(context.getErrors()[0].message).to.equal(
expect(validationErrors.length).to.equal(1);
expect(validationErrors[0].message).to.equal(
'The query exceeds the maximum complexity of 100. Actual complexity is 1000'
);
});
Expand All @@ -203,11 +205,11 @@ describe('QueryComplexity analysis', () => {
}
`);

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

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

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

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

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

const context = new ValidationContext(schema, ast, typeInfo);
const context = new ValidationContext(schema, ast, typeInfo, () => null);
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: [
fieldConfigEstimator(),
fieldExtensionsEstimator(),
simpleEstimator({
defaultComplexity: 1
})
Expand All @@ -353,19 +355,20 @@ describe('QueryComplexity analysis', () => {
requiredArgs
}
`);
const context = new ValidationContext(schema, ast, typeInfo);
const validationErrors: GraphQLError[] = [];
const context = new ValidationContext(schema, ast, typeInfo, err => validationErrors.push(err));
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: [
fieldConfigEstimator(),
fieldExtensionsEstimator(),
simpleEstimator({
defaultComplexity: 1
})
]
});
visit(ast, visitWithTypeInfo(typeInfo, visitor));
expect(context.getErrors().length).to.equal(1);
expect(context.getErrors()[0].message).to.equal('Argument "count" of required type "Int!" was not provided.');
expect(validationErrors.length).to.equal(1);
expect(validationErrors[0].message).to.equal('Argument "count" of required type "Int!" was not provided.');
});

it('should report error when no estimator is configured', () => {
Expand All @@ -374,14 +377,15 @@ describe('QueryComplexity analysis', () => {
scalar
}
`);
const context = new ValidationContext(schema, ast, typeInfo);
const validationErrors: GraphQLError[] = [];
const context = new ValidationContext(schema, ast, typeInfo, err => validationErrors.push(err));
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: []
});
visit(ast, visitWithTypeInfo(typeInfo, visitor));
expect(context.getErrors().length).to.equal(1);
expect(context.getErrors()[0].message).to.equal(
expect(validationErrors.length).to.equal(1);
expect(validationErrors[0].message).to.equal(
'No complexity could be calculated for field Query.scalar. ' +
'At least one complexity estimator has to return a complexity score.'
);
Expand All @@ -393,16 +397,17 @@ describe('QueryComplexity analysis', () => {
scalar
}
`);
const context = new ValidationContext(schema, ast, typeInfo);
const validationErrors: GraphQLError[] = [];
const context = new ValidationContext(schema, ast, typeInfo, err => validationErrors.push(err));
const visitor = new ComplexityVisitor(context, {
maximumComplexity: 100,
estimators: [
fieldConfigEstimator()
fieldExtensionsEstimator()
]
});
visit(ast, visitWithTypeInfo(typeInfo, visitor));
expect(context.getErrors().length).to.equal(1);
expect(context.getErrors()[0].message).to.equal(
expect(validationErrors.length).to.equal(1);
expect(validationErrors[0].message).to.equal(
'No complexity could be calculated for field Query.scalar. ' +
'At least one complexity estimator has to return a complexity score.'
);
Expand Down Expand Up @@ -463,7 +468,7 @@ describe('QueryComplexity analysis', () => {

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

const complexity2 = getComplexity({
estimators: [
fieldConfigEstimator(),
fieldExtensionsEstimator(),
simpleEstimator({defaultComplexity: 1})
],
schema,
Expand Down
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