Skip to content

Support graphql v15 #29

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

Closed
rigelglen opened this issue May 8, 2020 · 6 comments · Fixed by #30
Closed

Support graphql v15 #29

rigelglen opened this issue May 8, 2020 · 6 comments · Fixed by #30

Comments

@rigelglen
Copy link

rigelglen commented May 8, 2020

Hi, graphql-query-complexity currently does not include graphql 15.0 as a supported peer depenency. Even though graphql 15 came with some breaking changes, I don't think anything needs to be changed to support it.

@robhogan
Copy link
Contributor

robhogan commented May 22, 2020

Unfortunately not that simple - this lib makes heavy use of ValidationContext which has a breaking change for v15, no longer collecting its own errors. I'm looking into this now, but even with things naively patched up I'm seeing the tests failing, so I'm fairly certain this library will give the wrong complexity results with graphql 15.

Update: this turned out to be not quite true - the library actually worked fine without changes, except for the deprecated estimators. Most of the changes required were in test code. So if you’re stuck on 0.5.0 I don’t think there’s any reason to worry.

@ivome
Copy link
Collaborator

ivome commented May 22, 2020

There's already a PR #27 which addresses some issue with the ValidationContext. I haven't had time to look into this in detail yet. If someone wants to help with testing this and adding support for graphql v15, I'll be happy to review a PR.

@robhogan
Copy link
Contributor

robhogan commented May 22, 2020

The change to ValidationContext is easily dealt with but there's a bigger problem - non-standard properties from field objects have been removed, which means complexity is no longer available on fields on the parsed schema.

It looks like this means fieldConfigEstimator and legacyEstimator are doomed, but they are already deprecated anyway. fieldExtensionEstimator, directiveEstimator and simpleEstimator work fine.

Would you accept a PR which removes the deprecated/broken estimators, and updates the peer deps and dev deps for graphql to v15?

@ivome
Copy link
Collaborator

ivome commented May 22, 2020

The fieldConfigEstimator and legacyEstimator have been deprecated for quite a while with console warnings. So I'd say we can drop support for those in the upcoming release since you can do the exact same thing with the fieldExtensionEstimator (or bake your own).

If you want to create a PR for this, go ahead 👍

@robhogan
Copy link
Contributor

Great 👍 . PR now in: #30

@ivome
Copy link
Collaborator

ivome commented May 25, 2020

This is now supported in v0.6.0

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 a pull request may close this issue.

3 participants