Skip to content
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

Merged
merged 10 commits into from May 25, 2020

Conversation

robhogan
Copy link
Contributor

@robhogan robhogan commented May 23, 2020

  • Change uses of ValidationContext to use the onError constructor arg rather than the getErrors method. onError was added in 14.5.0 and made mandatory in 15.0.0, and getErrors was removed in 15.0.0.
  • Remove deprecated fieldConfigEstimator and legacyFieldEstimator. These can no longer be supported due to this breaking change in graphql
  • Update dev dependencies to graphql@^15.0.0 and remove @types/graphql (graphql has included its own types since 14.5.0).
  • Bump peer dependency to ^14.5.0 || ^15.0.0.

Obviously this PR contains breaking changes! :)

Closes #29

@robhogan robhogan mentioned this pull request May 23, 2020
Comment on lines 136 to 138
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

@@ -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

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

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?

@robhogan robhogan changed the title Compatibility with graphql ^14.5.0 || ^15.0.0 Compatibility with graphql ^15.0.0 May 25, 2020
@ivome ivome merged commit 60ab617 into slicknode:master May 25, 2020
@ivome
Copy link
Collaborator

ivome commented May 25, 2020

Thanks for adding this @rh389! I just merged this and published v0.6.0 on npm.

@robhogan
Copy link
Contributor Author

Thanks for being so responsive, and thanks for the library 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support graphql v15
2 participants