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

Factor out core to support HAPI, Express, Koa and others #11

Closed
nnance opened this issue May 26, 2016 · 25 comments
Closed

Factor out core to support HAPI, Express, Koa and others #11

nnance opened this issue May 26, 2016 · 25 comments

Comments

@nnance
Copy link
Contributor

nnance commented May 26, 2016

Add support for the HAPI web framework while continuing support for Express. The intent here is to offer support for both frameworks in a consistently or such that the API works well with each framework and detects which binding is being used.

I believe it is likely we will need an integration with HAPI much like express-graphql.

@helfer helfer changed the title Support HAPI and Express frameworks Factor out core to support HAPI, Express, Koa and others May 29, 2016
@nnance
Copy link
Contributor Author

nnance commented May 29, 2016

@helfer @bkinsey808 @TimMikeladze moving the architecture discussion from the typescript #8 discussion. As I mentioned before I have a prototype based on the following architecture:

  1. Apollo Server would be a class that would be instantiated with a schema and validation rules
  2. It would have a method called fulfillQuery that would parse, validate and execute the query returning an object that would contain an errors and or data

I now have a working version of my prototype. It is very naive and returns a hardcoded response but both express and hapi servers are running and responding GQL requests that is parsed and validated. It currently only works on posts with a query in the body.

Let me know what you think

@helfer
Copy link
Contributor

helfer commented Jun 1, 2016

I'm back from the long weekend and I gave this some thought. I think @nnance 's repo is a great start, so that can be our reference.

I'd like the core of apolloServer to simply be a function, which takes a few more parameters than the graphql function in graphql-js. That will allow for the most flexibility. The function signature should be something like this:

apolloServer({
  schema: GraphQLSchema,
  request: String || QueryAST,
  context?: any,
  rootValue?: any,
  variables?: {[key: String]: any},
  operationName?: String,
  validationRules?: [validationRule],
  logFunction?: function
 }) => Promise({
  data?: Object
  errors?: [error]
});

I don't really follow any specific notation here, but I think you get the idea. Instead of passing params to apolloServer, you could also pass a function, which returns a promise (similar to express-graphql).

Using a function over say an instance of a class has the advantage of staying more flexible without really incurring any cost.

request can be both a string or an AST. If it's a string, it gets parsed and validated, if it's an AST it does not get parsed, and does not get validated. It just gets executed. This lets apolloServer work for pre-stored queries as well (the wrapper would have to implement the support for that).

logFunction gets called by the server in specific places, and when errors occur. The user can supply this function to do things like print errors to stderr, debug stuff, etc.

All other parameters work the same as they do for graphql

The most important difference to the current apolloServer is that it does no longer take a schema definition in schema language and connectors. Instead, people will use graphql-tools to generate an executableSchema.

Let me know what you guys think. Would this cover your use cases?

@helfer
Copy link
Contributor

helfer commented Jun 1, 2016

On second thought, it might make sense to also take a formatError function, and a pretty option like express-graphql does, just to offload that from the different wrappers. But then again, it might also be extra cruft that's just not needed. Not sure what the right tradeoff is there. Maybe I should write that DESIGN.md file first that I've been talking about in #2

It definitely should not take a graphiql option. For that we should build a separate package, which lets you serve graphiql easily.

@bkinsey808
Copy link

Would somebody pls be willing to explain to me the plan for supporting databases? Will it support multiple databases? Nosql, relational? Will the graphql schema be autogen'd from the db schema?

@bkinsey808
Copy link

I'm also really interested in autogen-ing typescript types from graphql types, Is that possibly in the mix?

@helfer
Copy link
Contributor

helfer commented Jun 1, 2016

@bkinsey808 it's on our radar, but not on our short-term roadmap. If you want to discuss further, you can open a separate issue for each topic. :)

@helfer helfer mentioned this issue Jun 1, 2016
@nnance
Copy link
Contributor Author

nnance commented Jun 1, 2016

@helfer I like including the option for pre-stored queries

@HriBB
Copy link
Contributor

HriBB commented Jun 15, 2016

@helfer what is the status of this? How can I help? It would be really helpful if we could get some pointers. Unfortunately I haven't had the time to check @nnance prototype yet ...

@nnance
Copy link
Contributor Author

nnance commented Jun 15, 2016

@HriBB we are actively working on this issue. The new version will be completely rewritten in typescript and support at least 2 initial integrations (express and hapi). I will @helfer speak up on his thoughts when this will be available in master.

@helfer
Copy link
Contributor

helfer commented Jun 15, 2016

@HriBB we've made some good progress, you can see it on the core-refactor branch. We have a working & tested small core in typescript and are now adding express integration to it.

Nick is going to be working on the HAPI integration. If you want, you could get the express thing working. As a first step, we want it to reproduce the express-graphql functionality, and pass all the tests that express-graphql passes. Is that something you would be interested in working on? Or what are you most interested in?

@HriBB
Copy link
Contributor

HriBB commented Jun 19, 2016

@helfer I would love to help, but need to do some reading first ;) I can work on express and koa integrations.

I'm pretty busy these days, finishing some project, so I will have more time in a week or so.

@helfer
Copy link
Contributor

helfer commented Jun 20, 2016

@HriBB I think that's perfect timing. By then we'll have a working Express and HAPI integration I think, so you should be able to use those to guide a Koa integration.

@HriBB
Copy link
Contributor

HriBB commented Jun 22, 2016

@helfer this is my first try on koaApollo HriBB@f11a94f

Trying to implement this with npm link I am getting

ERROR [Error: Schema must be an instance of GraphQLSchema. Also ensure that there are not multiple versions of GraphQL installed in your node_modules directory.]

@helfer
Copy link
Contributor

helfer commented Jun 22, 2016

@HriBB That error usually happens when there's more than one version of GraphQL-js installed in your project. Are you using npm 2 by any chance? In that case you probably have to run npm dedupe to make sure you only have one version of GraphQL-js installed. npm list should tell you if you have multiple versions.

@HriBB
Copy link
Contributor

HriBB commented Jun 22, 2016

@helfer so I fixed the instance problem. Now I'm trying to create executable schema and pass it to the koaApollo(), but I'm getting this error

TypeError: connectors[connectorName] is not a function
    at /home/bojan/www/climbuddy/node_modules/graphql-tools/dist/schemaGenerator.js:185:39
    at Array.forEach (native)
    at attachconnectorFn (/home/bojan/www/climbuddy/node_modules/graphql-tools/dist/schemaGenerator.js:183:29)
    at /home/bojan/www/climbuddy/node_modules/graphql-tools/dist/schemaGenerator.js:358:18
    at /home/bojan/www/climbuddy/node_modules/graphql-tools/dist/schemaGenerator.js:294:16
    at /home/bojan/www/climbuddy/node_modules/graphql-tools/dist/schemaGenerator.js:315:17
    at resolveOrError (/home/bojan/www/climbuddy/node_modules/graphql/execution/execute.js:457:12)
    at resolveField (/home/bojan/www/climbuddy/node_modules/graphql/execution/execute.js:448:16)
    at /home/bojan/www/climbuddy/node_modules/graphql/execution/execute.js:256:18
    at Array.reduce (native)
    at executeFields (/home/bojan/www/climbuddy/node_modules/graphql/execution/execute.js:254:50)
    at executeOperation (/home/bojan/www/climbuddy/node_modules/graphql/execution/execute.js:196:10)
    at /home/bojan/www/climbuddy/node_modules/graphql/execution/execute.js:114:13
    at new Promise (/home/bojan/www/climbuddy/node_modules/core-js/modules/es6.promise.js:197:7)
    at Promise.exports.(anonymous function).target.(anonymous function).function.target.(anonymous function).F (/home/bojan/www/climbuddy/node_modules/babel-runtime/node_modules/core-js/library/modules/_export.js:35:28)
    at Object.execute (/home/bojan/www/climbuddy/node_modules/graphql/execution/execute.js:113:10)

koaApollo.js
https://github.com/HriBB/apollo-server/blob/4ead319493fffdffa6f7592fc1b67a0af47147af/src/integrations/koaApollo.ts

import {
  GraphQLSchema,
  GraphQLResult,
} from 'graphql';

import { runQuery } from '../core/runQuery';

//import * as GraphiQL from '../modules/renderGraphiQL';

export interface KoaApolloOptions {
    schema: GraphQLSchema;
    formatError?: Function;
    rootValue?: any;
    context?: any;
    logFunction?: Function;
}

export function koaApollo(options: KoaApolloOptions): GraphQLResult {
  if (!options) {
    throw new Error('Apollo koa middleware requires options.');
  }
  if (arguments.length > 1) {
    throw new Error(`apolloServer expects exactly one argument, got ${arguments.length + 1}`);
  }
  return function *() {
    try {
      const query = getQueryString(this.request);
      const gqlResponse = yield runQuery({
        schema: options.schema,
        query: query,
        context: options.context || {},
      });
      this.body = gqlResponse;
    }
    catch (e) {
      this.body = { errors: [e.stack] };
    }
  };
}

function getQueryString(req) {
  if (req.method === 'POST') {
    return req.body.query;
  } else if (req.method === 'GET') {
    return req.query.query;
  }
  throw new Error(`HTTP method ${req.method} not supported`);
}

and my server.js

import koa from 'koa'

// ...

import {
  makeExecutableSchema,
  buildSchemaFromTypeDefinitions,
  addErrorLoggingToSchema,
  addCatchUndefinedToSchema,
  addResolveFunctionsToSchema,
  attachConnectorsToContext,
} from 'graphql-tools';

import schema from './data/schema'
import resolvers from './data/resolvers'
import connectors from './data/connectors'

const app = koa()

const executableSchema = makeExecutableSchema({
  typeDefs: schema,
  resolvers,
  connectors,
  allowUndefinedInResolve: true,
});

addErrorLoggingToSchema(executableSchema, { log: (e) => console.error('==> ERROR', e.stack) });

// server graphql
app.use(route.post('/graphql', koaApollo({
  schema: executableSchema,
  context: {}
})))

@helfer
Copy link
Contributor

helfer commented Jun 22, 2016

Are your connectors classes with a constructor that takes the context as an argument?

@HriBB
Copy link
Contributor

HriBB commented Jun 22, 2016

Nope, my connectors are sequelize models. OK, so I guess I need to convert my connectors to functions?

@helfer
Copy link
Contributor

helfer commented Jun 22, 2016

Yeah, the connectors are something we came up with a while ago, but they've changed shape a little bit. My suggestion would be to remove the connectors from your call to makeExecutableSchema and instead stick the sequelize models on the schema by hand, or if you don't care about per-request caching and batching, just import them directly in the resolvers, like I did for the Apollo server tutorial.

@HriBB
Copy link
Contributor

HriBB commented Jun 22, 2016

@helfer It works! Thanks for your help.

@helfer
Copy link
Contributor

helfer commented Jun 29, 2016

For the current status of this, see #41

@helfer
Copy link
Contributor

helfer commented Jul 7, 2016

@HriBB now might be a good time to start a Koa integration. The Express integration is done, and @nnance is getting close to finishing the HAPI integration! :)

@HriBB
Copy link
Contributor

HriBB commented Jul 8, 2016

@helfer yes I've been monitoring the progress and will start next week. From what I have seen it should not take me too long to have a working and tested implementation. I already have one with the slightly older version and without graphiql.

@helfer
Copy link
Contributor

helfer commented Jul 27, 2016

@HriBB are you still interested in working on an implementation for Koa?

@helfer helfer reopened this Jul 27, 2016
@HriBB
Copy link
Contributor

HriBB commented Jul 27, 2016

@helfer yes, trying to find some time to work on this ... today looks rainy enough :)

@helfer
Copy link
Contributor

helfer commented Jul 29, 2016

Think this is complete now with #28 #46, #58, #59 having all been merged

@helfer helfer closed this as completed Jul 29, 2016
trevor-scheer pushed a commit that referenced this issue May 6, 2020
…istered

op-reg: Only forbid operations if `forbidUnregisteredOperations` is set.
trevor-scheer pushed a commit that referenced this issue May 14, 2020
…istered

op-reg: Only forbid operations if `forbidUnregisteredOperations` is set.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants