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

Deprecate built-in GraphQL support #1135

Merged
merged 3 commits into from Feb 5, 2021
Merged

Deprecate built-in GraphQL support #1135

merged 3 commits into from Feb 5, 2021

Conversation

JayH5
Copy link
Member

@JayH5 JayH5 commented Feb 3, 2021

See #619

I'm trying to do some house-keeping so that we can better maintain Starlette. We need to either put some major work into the GraphQL integration (upgrade to Graphene 3, add a bunch of stuff people want), or deprecate it. I'm in favour of the latter, since there doesn't seem to be much interest in maintaining it among @encode/maintainers (myself included).

There are currently 11 open issues and 9 pull requests related to GraphQL. Meanwhile, the last real change to GraphQLApp was in October 2019 (#623).

FastAPI does advertise GraphQL support via Starlette, but as far as I can tell doesn't actually integrate with it in any way.

This is a proposal and intended just to hopefully get the ball rolling and garner some discussion.

@@ -36,6 +36,7 @@ markdown_extensions:
- markdown.extensions.codehilite:
guess_lang: false
- mkautodoc
- admonition
Copy link
Member Author

Choose a reason for hiding this comment

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

This is to get the nice warning message to render:
Screenshot 2021-02-03 at 22 00 21

@florimondmanca
Copy link
Member

florimondmanca commented Feb 3, 2021

Sounds like a clear yes to me. There are already several GraphQL specific projects that support ASGI (Strawberry, Ariadne, Tartiflette) (edit: just like you wrote in the docs :)), which makes them trivial to integrate within a Starlette based app (eg via submounting). Definitely better let specialized libraries deal with GraphQL which is an entire world by itself.

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Not sure what's left to figure out to get this merge able but what's here looks good to me. :)

@erewok
Copy link
Contributor

erewok commented Feb 3, 2021

I agree that this is the right direction to go in and I support this proposal.

@JayH5 JayH5 marked this pull request as ready for review February 4, 2021 10:21
@JayH5
Copy link
Member Author

JayH5 commented Feb 4, 2021

Cool, thanks. I guess I had two things in mind before merging this:

  1. Possibly refining the warning messages a bit.
  2. Making sure the right people know about this.

So for 2):

  • @tiangolo hi 👋 if you're around, for if/when FastAPI updates its Starlette dependency, this is something to keep in mind.
  • @ciscorn hey 👋 I've linked your project in the docs here. As one of the smaller projects listed I just wanted to make sure that's okay with you? 🙂

@ciscorn
Copy link
Sponsor Contributor

ciscorn commented Feb 4, 2021

@JayH5 No problem. Thank you! 🙂

@Kludex
Copy link
Sponsor Member

Kludex commented Feb 4, 2021

I just want to say that I'm happy with this approach and that makes a lot of sense on doing it.

Also, if this approach is followed the same way in the future, it will be cool. I'm saying this because the removal of UJSONResponse which represented a breaking change (even if it's easy to implement).

@rafalp
Copy link
Member

rafalp commented Feb 4, 2021

Do it

@JayH5
Copy link
Member Author

JayH5 commented Feb 4, 2021

I've tweaked the warning messages so that the Starlette version is mentioned in the docs rather than the code (since the code is already versioned but the docs are not).

Thinking I'll keep #619 open until GraphQLApp is actually removed. Will merge this soon unless any last objections 🙂

@JayH5 JayH5 changed the title Proposal: Deprecate built-in GraphQL support Deprecate built-in GraphQL support Feb 4, 2021
@JayH5 JayH5 added the graphql label Feb 4, 2021
@JayH5 JayH5 merged commit 3274594 into master Feb 5, 2021
@JayH5 JayH5 deleted the deprecate-graphql branch February 5, 2021 19:13
@lightoyou
Copy link

@tiangolo can you give recommandation ? Are you going to integrate something inside fast-api ? or let's people choose ?

@mecampbellsoup
Copy link

@tiangolo can you give recommandation ? Are you going to integrate something inside fast-api ? or let's people choose ?

#1135 (comment)

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.

None yet

8 participants