wagtail-grapple icon indicating copy to clipboard operation
wagtail-grapple copied to clipboard

Upgrade graphene to v3

Open fabienheureux opened this issue 4 years ago • 6 comments

Graphene v3 is almost there...since this summer. In fact the release is blocked by several things:

  • some packages of graphene ecosystem are almost abandonned
  • Graphene team wants every package of the ecosystem to be ready before releasing an official graphene v3 upgrade
  • The maintainer (jkimbo) who did a great job tracking graphene v3 changes left a few days ago

So this is more likely we are a bit far from getting an official graphene v3 release, but I use it on production on a django / graphene project without major issues, and this project does not use dataloaders or other fancy features from graphene (that are poorly supported in graphene v3), so the upgrade should be really safe.

  • [x] This PR https://github.com/GrappleGQL/wagtail-grapple/commit/ad1ad1961b1c7da9fc117be49d0f9b2c8b509830 introduced a dependency to an error class that was removed from graphql-core https://github.com/graphql-python/graphql-core/blob/master/src/graphql/error/located_error.py#L11
  • [x] This PR https://github.com/GrappleGQL/wagtail-grapple/commit/bb354c9ae7d2453ffbf0258c873071ccb7373206 introduced a dependency to a validation class that was renamed in graphql-core https://github.com/graphql-python/graphql-core/blob /7c72a38af6bc74ff7919b1ccd3a76db4acad5f4e/src/graphql/validation/rules/no_unused_fragments.py#L10
  • [x] The hack that removes NoUnusedFragmentsRule validator from the query validation now throws an error ( https://github.com/GrappleGQL/wagtail-grapple/blob/master/grapple/schema.py#L19 By the way, this does not follow GraphQL spec, so we could maybe plan on update this code ? Any idea @NathHorrigan @zerolab ? Spec for reference https://spec.graphql.org/June2018/#sec-Fragments-Must-Be-Used
  • [ ] Add changelog about upgrade considerations (check things that can break etc)
  • [ ] Run a few benchmarks in order to justify this change (I noticed a big impact with long lists of data in my django / graphene project, see here https://twitter.com/jayden_windle/status/1235323199220592644)
  • [x] @register_singular_query_field function is broken
  • [x] @register_query_field function is broken

fabienheureux avatar Nov 23 '20 08:11 fabienheureux

The hack that removes NoUnusedFragmentsRule validator

This hack is not compliant with the spec but doesn't break anything per se. This originally came about when we first writing the preview compatibility with Gatsby. I know more about the GraphQL AST now and could probably write a parser that only imports the one that are used TBH (Obviously this would be taken care of in the Gatsby package).

NathHorrigan avatar Nov 24 '20 09:11 NathHorrigan

The hack that removes NoUnusedFragmentsRule validator

This hack is not compliant with the spec but doesn't break anything per se. This originally came about when we first writing the preview compatibility with Gatsby. I know more about the GraphQL AST now and could probably write a parser that only imports the one that are used TBH (Obviously this would be taken care of in the Gatsby package).

I currently catch the FrozenError and do nothing

fabienheureux avatar Dec 10 '20 08:12 fabienheureux

Tests finally pass :tada:

It would be nice to have a review at this stage (@zerolab @ruisaraiva19 @NathHorrigan).
I am in the process of adding benchmarks but I don't think these should block the code review. Thanks !

fabienheureux avatar Feb 07 '21 02:02 fabienheureux

To follow up on this: https://github.com/graphql-python/graphene/pull/1300 There is an open PR on Graphene repo related to the issue with None being passed into the kwargs when no value is provided.

fabienheureux avatar Feb 17 '21 06:02 fabienheureux

graphene-django v3.0.0 was just released, which adds Django 4 support.

https://github.com/graphql-python/graphene-django/releases/tag/v3.0.0

Relevant PR: https://github.com/graphql-python/graphene-django/pull/1281

via https://github.com/torchbox/wagtail-grapple/issues/241#issuecomment-1260243628 /ht @PeterDekkers


@fabienheureux any chance you have the time to look at the final tweaks here?

zerolab avatar Sep 28 '22 08:09 zerolab

graphene-django v3.0.0 was just released, which adds Django 4 support.

https://github.com/graphql-python/graphene-django/releases/tag/v3.0.0

Relevant PR: graphql-python/graphene-django#1281

via #241 (comment) /ht @PeterDekkers

@fabienheureux any chance you have the time to look at the final tweaks here?

Thanks for the nudge @zerolab, I will have some time in about ten days to finish this up.

fabienheureux avatar Sep 28 '22 12:09 fabienheureux

@fabienheureux Do you have any time to pick this up? I'm in the midst of implementing some federated fields to another api and I would really like to use gql >=3 with aiohttp support, but I'm getting a conflict with graphql-core. I'm happy to look into it as well if you're busy on other things.

dopry avatar Dec 17 '22 12:12 dopry

@fabienheureux I did some work on your branch and pushed it to my fork. see: https://github.com/dopry/wagtail-grapple/commits/feature/graphene-v3-upgrade

dopry avatar Dec 17 '22 13:12 dopry

Will close this in favour of #285 @fabienheureux thank you for chipping away at it. @dopry cheers for picking it up

zerolab avatar Dec 19 '22 14:12 zerolab

Thanks a lot for picking this up @zerolab, sorry got a bit busy with client work lately.

fabienheureux avatar Dec 20 '22 07:12 fabienheureux