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

Export graphqlKoa #2244

Closed

Conversation

KevinGrandon
Copy link

@KevinGrandon KevinGrandon commented Jan 30, 2019

Being able to access graphqlKoa is useful for koa-based frameworks that don't expose the app object. This was previously exported in an earlier version of apollo-server and would be useful to export in the 2.x release as well.

Here's an example of how we're currently consuming this: https://github.com/fusionjs/fusion-plugin-apollo-server/blob/90e95ef1480659073c7e4e042b75ea6205cc068f/src/server.js#L37

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

Being able to access graphqlKoa is useful for koa-based frameworks that don't expose the app object. This was previously exported in an earlier version of apollo-server and would be useful to export in the 2.x release as well.

Here's an example of how we're currently consuming this: https://github.com/fusionjs/fusion-plugin-apollo-server/blob/90e95ef1480659073c7e4e042b75ea6205cc068f/src/server.js#L37
@martijnwalraven
Copy link
Contributor

@KevinGrandon The problem with exporting graphqlKoa (and other integrations/middlewares) is that this calls runHttpQuery directly instead of going through ApolloServer (in apollo-server-core). Among other things, that circumvents schema construction options and the proper configuration of features like caching, persisted queries, Engine reporting, etc.

I realize there is a need to expose integration points with frameworks like Koa, but with the current structure I'm afraid this isn't the way to do it.

If applyMiddleware doesn't work for you, maybe there is another way to expose what you need from ApolloServer (in apollo-server-koa).

@vladshcherbin
Copy link
Contributor

@KevinGrandon hey, man. If would be awesome to get the old way of doing things in apollo koa. The linked issue already has > 50 upvotes, no response or interest from core team though. Maybe you'll be lucky to change this 😉🙏

@martijnwalraven
Copy link
Contributor

@vladshcherbin Have you read my reply above?

@vladshcherbin
Copy link
Contributor

@martijnwalraven yeah, I've been tracking this change since v2 koa PR and finally I see the answer to it from core team member for the first time in several months.

Hopefully, it'll be possible to export old middlewares with necessary changes (in koa or in core).

@nfantone
Copy link

Among other things, that circumvents schema construction options and the proper configuration of features like caching, persisted queries, Engine reporting, etc.

@martijnwalraven Care to elaborate on this, please? To me, these assertions seem more like forced, conscious design choices on your front and less than
technical impossibilities. v1 allows all of those features you brought up while still keeping middlewares exposed.

@martijnwalraven
Copy link
Contributor

martijnwalraven commented Jan 30, 2019

@nfantone Of course these are design decisions, and they are more opinionated than before. There are good reasons for those changes however.

Unlike the middleware functions exposed in v1, ApolloServer is meant to make server construction easier and allows us to keep internal state that enables performance improvements. We can rely on the schema remaining the same for example, which allows us to keep a document cache and avoid repeated parsing and validation (v1 didn't have caching, persisted queries or built-in Engine reporting).

That doesn't mean I'm happy with the current integration API. We actually want future versions of Apollo Server to be less dependent on existing frameworks, by making the integration API more lightweight. This could very well work by exposing middlewares, as long as these have access to an ApolloServer instance (which would no longer be integration-specific).

The current graphqlKoa function isn't a suitable API for the design direction we chose, but something like koaApolloServer(apolloServer) seems like a fine alternative. Do you agree with that?

@ericclemmons
Copy link

I think the problem we're having is that creating a server is simple in Node.

The expectation is that Apollo server can work as a (req, res) function, which composes well.

Perhaps what we're not understanding is why these features prevent a middleware signature?

@martijnwalraven
Copy link
Contributor

The expectation is that Apollo server can work as a (req, res) function, which composes well.
Perhaps what we're not understanding is why these features prevent a middleware signature?

None of this prevents a middleware signature! In fact, I strongly prefer a simple (req, res) function as our basic integration point, and I think the current approach of trying to configure a middleware stack (or equivalent) for each integration is inflexible and unmaintainable.

Exposing graphqlKoa in its current form won't do however, because that circumvents the initialization and configuration we do in ApolloServer completely, which leads to broken features.

One strong requirement of whatever API we land on is that we need to be able to keep shared internal state between requests, to avoid repeated initialization of persisted queries and other configuration, to cache parsed documents, to keep an open connection with a Memcache cluster for data source and response caching, etc.

To make this more concrete, a possible API for 3.0 might look like:

import { GraphQLService } from "apollo-server";
import { graphqlEndpoint } from "apollo-server-koa";

const service = new GraphQLService(...)

const app = new Koa();
app.use(graphqlEndpoint(service));

@nfantone
Copy link

nfantone commented Jan 31, 2019

@martijnwalraven Well... that last snippet looks pretty close to v1, doesn't it? Unless graphqlEndpoint would abstract away multiple responsibilities, I'd really like to see more of that in the future. Personally, I think the main reason (some) people strongly disagree with your current direction and design is that we lost modularity and composability - which is a flagship feature of middlewares and common among Node.js's web frameworks. applyMiddleware just does too many things, limiting customisation, imposing a rigid middleware order without too much room for configuration, which goes against that very original philosophy and feels unnecesary constrictive.

Unlike the middleware functions exposed in v1, ApolloServer is meant to make server construction easier.

I keep reading this argument and I honestly fail to see a compelling, solid reason behind it to support your design approach. I'd like to refer you to a comment I made a while back addressing exactly this and expanding on what I outlined in my previous paragraph. Here's an abstract:

But right there is just the thing: was that [constructing a server] ever a pain point, to begin with? Setting up basic subscriptions or most other extensions is -luckily- pretty straightforward and documentation is already available. Unsurprisingly, these kind of things usually revolve around setting up a bunch of new middlewares in your app.

Also,

v1 didn't have caching, persisted queries or built-in Engine reporting.

Apart from built-in Engine, a private service behind a paywall, v1 had (and still has) all those things.

@martijnwalraven
Copy link
Contributor

@nfantone I'm all with you on applyMiddleware, and I'm not a fan of the general idea of exporting ApolloServer entry points for each integration.

If your objective is to integrate with an existing framework like Koa, I think you're right we can expect you to set up your own middleware stack, and you do need a certain amount of flexibility in that. A lightweight function like graphqlEndpoint seems like a better integration point than applyMiddleware for that.

You shouldn't extrapolate too much from your own experience however. You'd be surprised how much of a stumbling block configuring a server has been for people (that was the main reason people started using GraphQL Yoga for example), so that's why we also want to offer an easy way to construct a stand alone ApolloServer, without having to choose between frameworks and without the need to configure something like a middleware stack manually.

The difference between the old middleware functions and the proposed graphqlEndpoint function is that I want this function to rely on a well encapsulated notion of a GraphQLService. Like I said before:

One strong requirement of whatever API we land on is that we need to be able to keep shared internal state between requests, to avoid repeated initialization of persisted queries and other configuration, to cache parsed documents, to keep an open connection with a Memcache cluster for data source and response caching, etc.

Your statement that v1 had all those things is simply false: while we offered persisted queries and caching, that relied on putting our closed source native proxy in front of your Apollo Server. What we did with v2 was to actually move these features into open source and make them available to everyone.

I'm the first to admit that we made mistakes in the process, but I hope we can bring back some of the flexibility we lost while keeping the added convenience and efficiency a more opinionated design gives you.

By encapsulating schema construction and GraphQL-specific request processing into a class like GraphQLService, while keeping the interface with existing server frameworks lightweight, I believe we can give you the best of both worlds.

Another reason for this separation is that development and tooling workflows often require you to initialize a GraphQL service without starting up an entire server (or without starting up the same type of server that is used in production). Cleanly separating the GraphQL service from the server environment that it runs in gives us more flexibility in how services are used across development and deployment.

@vladshcherbin
Copy link
Contributor

It's okay if GraphQLService class would be passed into middlewares, similar to how config options were passed before (schema, context, etc), just don't add there routing stuff, cors, body-parser, etc. Make it a lightweight class with lightweight middlewares, which can be plugged into router as before - this way it won't feel like an alien in our apps. Something similar to this:

const graphQLService = new GraphQLService(...)

router
  .get('/graphql', graphqlPlayground(graphQLService))
  .post('/graphql', koaBody(), graphqlEndpoint(graphQLService))

For the other part of devs you can always export a full-featured, zero-config ApolloServer with everything configured for you (similar to apollo-boost).

So, if the path is kinda clear, the real question is when should we wait for this changes? While ApolloServer option already exists for users who like it and want to use, the other part is still stuck with v1 which is 7 month old and has old versions of everything.

@martijnwalraven
Copy link
Contributor

@vladshcherbin Glad to hear you like the direction! I'm hoping we can make progress on this soon. Our plan is to get the core GraphQLService API in place over the next few weeks, so we could start working on the integrations right after.

@nfantone
Copy link

nfantone commented Jan 31, 2019

Exactly what @vladshcherbin said.

@martijnwalraven Allow me to intertwine both your answers.

You shouldn't extrapolate too much from your own experience however. You'd be surprised how much of a stumbling block configuring a server has been for people [...]

I agree on the extrapolation part. I probably shouldn't. The development ecosystem is vast and inviting newcomers and providing good initial experiences is a great goal to strive for. But the fact remains that by the time I learned about the new v2 API and came to this very repository, there were already people raising their concerns about the proposal - even before the original PR for a Koa implementation was merged. Today, while there isn't a single congruent place for this, #1308 (a petition to re-export middlewares) is one of the top ranked issues in the apollo-server repository with 50+ upvotes. And it has received barely any attention from contributors or team members. So while my personal experience, in isolation, is not very representative, I feel like the combination of all of the above facts really are.

Your statement that v1 had all those things is simply false

Respectfully, I don't agree. I am currently running two apollo-server-koa 1.4.0 in production, on different projects, with all those features. You even said so yourself: you can enable caching and persisted queries in v1. It may be somewhat more involved, and not "built-in" - yes. But that's exactly what this present discussion is all about.

(from @vladshcherbin)

For the other part of devs you can always export a full-featured, zero-config ApolloServer with everything configured for you (similar to apollo-boost).

I feel like this would be a smart approach. Build stuff from middlewares and export an optional fully featured server instance, already configured, constructed off of them. I'd love to see an approach where enabling/disabling features would require declaring a middleware:

router
  .post(
    '/graphql',
    koaBody(),
    graphqlEndpoint(graphQLService),
    graphqlCache(),
    graphqlUpload()
    /*, ... */
);

@alexFaunt
Copy link

Jumping over from being a long time watcher of #1308. I just want to say I agree with everything @nfantone and @vladshcherbin are asking for here. The suggested API above of

const service = new GraphQLService(...) 
const app = new Koa(); 
app.use(graphqlEndpoint(service));

is all I would need to see to upgrade our current production servers from v1, and I think the apollo-boost analogy is a perfect way to address newcomers in setting up quickly.

Being stuck on v1 is causing us several issues and i'm confident we are not the only ones, apologies for being largely a big fat +1 to this, but I'm excited to see some agreement, progress and discussion of the future API 👍

@KevinGrandon
Copy link
Author

Obsoleted by #2435.

@abernix
Copy link
Member

abernix commented Aug 23, 2019

#2435 finally landed in Apollo Server 2.9.0. 😄

Copying and pasting my comments from #1308 (comment):

Ok, after recovering from the need to revert it, the getMiddleware bit that I suggested (just) above in #1308 (comment) finally landed in Apollo Server 2.9.0 (thanks to #3047). (Finally!)

That said, I've made a proposal for what I hope is an even better and more flexible/customizable solution in the future. Those subscribers to this issue seem like a relevant audience to solicit for input, so please do take a look at the proposal I put together in #3184! 😄

(For now, I hope getMiddleware will help y'all.)

@annymosse
Copy link

@abernix is there a guide to create integration with custom framework ?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants