-
Notifications
You must be signed in to change notification settings - Fork 41
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.
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
Compatibility with graphql ^15.0.0 #30
Conversation
….0 || ^15.0.0 (onError was introduced in 14.5.0)
…ator. Remove fieldConfigEstimator and legacyEstimator.
src/QueryComplexity.ts
Outdated
this.estimators = options.estimators || [ | ||
legacyEstimator(), | ||
simpleEstimator() | ||
]; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -20,7 +20,7 @@ | |||
"lodash.get": "^4.4.2" | |||
}, | |||
"peerDependencies": { | |||
"graphql": "^0.13.0 || ^14.0.0" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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. - 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.
- Bump peer dependency up to >= 14.5
Your call sir!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
…rt the older `getErrors` API.
^14.5.0 || ^15.0.0
Thanks for adding this @rh389! I just merged this and published v0.6.0 on npm. |
Thanks for being so responsive, and thanks for the library 👍 |
ValidationContext
to use theonError
constructor arg rather than thegetErrors
method.onError
was added in14.5.0
and made mandatory in15.0.0
, andgetErrors
was removed in15.0.0
.fieldConfigEstimator
andlegacyFieldEstimator
. These can no longer be supported due to this breaking change ingraphql
graphql@^15.0.0
and remove@types/graphql
(graphql
has included its own types since14.5.0
).^14.5.0 || ^15.0.0
.Obviously this PR contains breaking changes! :)
Closes #29