Integration issues

[kbrandwijk]

After having a good look at the getting started and the sources, I'd like to share a few integration issues that I've come across. Basically the biggest issue is the fact that the output from gramps() goes directly into the apollo-server middleware. I thought I could get around it by accessing the individual properties of that object (for example, the schema), but that's also not possible, because the output is actually a function of req. Of course, I can work around that as well, by running gramps()(null).schema to get to the schema, but that doesn't really feel right.

This means that I end up with an all or nothing solution, which is not really what I would like. I'd like to have the ability to composition these components into my own server.

To be able to do this I need the following:

• Access to the resulting schema. That makes it easier to use that and apply my own stitching, or add additional schemas to my server outside of GrAMPS.
• Access to a middleware function that constructs my context. Currently, the context is only passed in to apollo-server. Ideally, the context would be constructed in an Express middleware function, so I have the possibility to add my own required bits and pieces to the context as well, before it is passed in to apollo-server.

This might be as easy as:

const gramps = gramps()
const schema = gramps.schema

// do my own stuff with my schemas

app.use('/graphql', gramps.setContext())

// do my own stuff in the context

app.use('/graphql', graphqlExpress({ schema: finalSchema, context: req => req, /* additional options */ })

Now, if I didn't need all of that, I would still be able to do:

const gramps = gramps()
app.use('/graphql', express.json(), graphqlExpress(gramps.serverOptions));

So it wouldn't impact the getting-started experience much, but would make it a lot easier to either use it in a more mixed environment, or integrate it with other tools (like graphql-yoga and my upcoming supergraph framework).

[jlengstorf]

I think we could make this work, yeah.

For default cases, I think GrAMPS working standalone makes sense, so I don't think I'd want to change that API (especially because breaking changes this early on seem ill-advised).

However, I think we could easily abstract out the core parts of GrAMPS into named exports, so you could do something like this:

import { generateExecutableSchema } from 'gramps';

const gramps = generateExecutableSchema();

const schema = gramps.schema;

// do whatever you want

app.use('/graphql', graphqlExpress({
  schema: finalSchema,
  context: req => ({
    req,
    gramps: gramps.context(req),
  }),
  // additional options...
}));

Were you thinking the setContext() method in your example would be middleware to attach the context to req? Or something else?

[jlengstorf]

^^ to clarify, generateExecutableSchema would effectively return the same GraphQLOptions object as the default GrAMPS export, but wouldn't wrap it with the req => /* ... */ function.

[kbrandwijk]

And where would I pass in my GrAMPS datasources in this example?

Also, as the function does not return just the executable schema, maybe you can call it prepare()?

[kbrandwijk]

Regarding the context, that depends on where you 'need' it to be. I see in your example you pass it in on the top level, so you would have (in apollo) context.req and context.gramps. I usually pass in the entire req object as context, so if I add in to req in an Express middleware, it would still end up in context.gramps for apollo, which is good.

Maybe you could expose an actual middleware function too, so I don't have to do:

app.use((req, res, next) => { 
   req.gramps = gramps.context(req)
   next()
}

But could instead do:

app.use(gramps.contextMiddleware)

[jlengstorf]

prepare() makes sense, yeah.

It would have the same API as gramps(), I imagine, with the limitation that it doesn't have access to req for extraContext.

We could refactor under the hood so that gramps() calls prepare() to make sure we keep things DRY.

Data sources scope context in resolvers to their own namespace + the content of extraContext, so we'd have to think through that. Basically anything that a GrAMPS data source needs to access will need to be attached via:

const gramps = prepare({
  dataSources: [/* ... */],
  extraContext: {
    // any extra context needed by GrAMPS data sources goes here
  },
});

[kbrandwijk]

See my proposal (very initial version, no linting, tests, or even running). I don't think there's an issue with extraContext.

[jlengstorf]

At first glance, this looks like it's the right direction. @ecwyne, anything seem out of place with the API in #63?

[kbrandwijk]

So, to recap, that would mean you could alternatively do (keeping the existing API 100% intact):

import { prepare } from 'gramps'

const gramps = prepare({ /* all gramps() parameters here */ })
const schema = gramps.schema

// do my own stuff with my schemas

app.use('/graphql', gramps.contextMiddleware)

// do my own stuff in the context

app.use('/graphql', graphqlExpress({ schema: finalSchema, context: req => req, /* additional options */ })

Looking at this code sample, app.use('/graphql', gramps.context) or app.use('/graphql', gramps.addContext()) might be easier to read.

[jlengstorf]

This seems most descriptive to my eye:

app.use('/graphql', gramps.addContext);

As long as the existing API stays intact, I think this is a solid addition for advanced use cases.

[kbrandwijk]

As add is a verb, I would go for gramps.addContext(), not gramps.addContext, but that's just semantics.

[kbrandwijk]

I threw together a quick boilerplate, for use with graphql-cli. Run the following command to create a server:

graphql create server -b https://github.com/supergraphql/gramps-boilerplate.

As you can see in the resulting index file, this now integrates perfectly with the existing (best practice) server boilerplates and the other tools.

[jlengstorf]

This is awesome. Thank you!

[ecwyne]

This is a totally different take on a solution, but let me know what you think @kbrandwijk. If what you need is the resulting schema and the getContext function created by GrAMPS, what if we just add those as properties to the middleware function returned by GrAMPS.

// gramps.js
const middleware = req => ({
    schema,
    context: getContext(req),
    ...apolloOptions.graphqlExpress,
  });
  middleware.schema = schema;
  middleware.getContext = getContext;
  return middleware;

Then you can use them however you'd like.

import gramps from 'gramps'

const middleware = gramps({ /* all gramps() parameters here */ });
const { schema, getContext } = middleware;

// do my own stuff with my schemas

app.use('/graphql', middleware)

// do my own stuff in the context

app.use('/graphql', graphqlExpress({ schema: finalSchema, context: req => req, /* additional options */ })

[kbrandwijk]

@ecwyne Although technically allowed in JS, the signature of what gramps() returns is a bit awful like this. I don't like functions with additional properties.

Also, for type safety in TS, it would result in a typing declaration that uses declaration merging in order to work, which I also try to avoid usually.

function middleware(req: express.Request): ApolloServerOptions { }
namespace middleware {
    // schema and getContext here;
}

[jlengstorf]

I'm inclined to agree: adding props to functions has always looked hacky to me, and I think it increases the cognitive overhead of understanding what's going on.