Add support for adding `response` to the context on custom Express server

[robin-macpherson]

What

• Currently, we cannot set up the express server so that we can access the response from inside of resolvers.

Why

• It's important to support this so that users can do things like -- in my current use case -- access the cookie on the response inside of resolvers, which could be important for a lot of basic authentication setups, for example with Jason Web Tokens as I am doing.

How

• We should add support so that we can pass the response as a second parameter in schema.addToContext along with the request, with an implementation that would look something like:

schema.addToContext((request, response) => ({
  request,
  response,
}))

[jasonkuhrt]

@robin-macpherson can you share a link to or snippet here of the use-case. I want to better understand if middleware should or should not play a role here for example.

[robin-macpherson]

@jasonkuhrt absolutely.

Repo & relevant branch

Relevant files are:

• backend/src/app.ts for server setup
• backend/src/graphql.ts where you can look at the resolver inside the createUser mutation starting on line 64.

Does that give you enough context?

[jasonkuhrt]

Thanks @robin-macpherson, yep (same thing I had checked out the other day IIUC).

So, I don't think addToContext api should get larger. I actually think it should get smaller #509.

The need is that, after a particular resolver runs, a header must be set on the response.

The API we design here cannot assume that Express server will be present. One plugin could contribute schema fields while another swaps the server (Fastify instead of Express).

We could add to core a setResponseHeader function to ctx. Or ctx.response.setHeader. Custom servers would then have to satisfy this interface (easy).

A simple setHeader api is pretty low level. cookie being one example where things get complex. That's why functional helpers exist to construct them. This leads me to think we're better with ctx.response namespace so we can add things like ctx.response.setCookie.

If we're going to have ctx.response then I think we should consider ctx.request. But I don't think request should be an API here, but rather just some data. Probably the same one available in addToContext and being sketched in #509.

Considering that we should abbreviate to ctx.req/ctx.res. Most users will write e.g. (root, args, ctx, info) not (root, arguments, context, info). Likewise I think most users will write addToContext(req => { .... Aligning the properties with how users call their arguments seems nice. Another example of abbreviation is we say ctx.db not ctx.database. And jwt not jsonWebToken.

Example

      resolve: async (parent, args, ctx) => {
        const Password = await bcrypt.hash(args.Password, 10)
        const user = await ctx.db.user.create({
          data: {
            Name: args.Name,
            Email: args.Email.toLowerCase(),
            Password,
          },
        })
        const token = jwt.sign({ userId: user.Id }, process.env.APP_SECRET!)
        ctx.res.cookie({
          name: 'token',
          value: token,
          httpOnly: true,
          maxAge: ONE_YEAR,
        })
        return user
      },
    })

And no longer needed:

-schema.addToContext((request: any) => ({
-  request,
-  response: request.response,
-}))

[robin-macpherson]

@jasonkuhrt that definitely seems like a fair point regarding the addToContext API getting too large, per your thoughts in #509 . I also agree that the ctx.response namespace would be better and less confusing. I do find it very confusing to have the response on the request.

Regarding abbreviations, can the base property name still be the unabbreviated version which could then be aliased to the abbreviated names that you described, so that we can support both? Perhaps my mixed feelings about making the property names themselves abbreviated just comes from my weird style of keeping things super clear and explicit so I often actually do write context and request, response, etc.

It just feels like it would be nice to support both but perhaps this just makes the whole thing more convoluted. Curious to know your thoughts.

[jasonkuhrt]

Regarding abbreviations, can the base property name still be the unabbreviated version which could then be aliased to the abbreviated names that you described, so that we can support both? Perhaps my mixed feelings about making the property names themselves abbreviated just comes from my weird style of keeping things super clear and explicit so I often actually do write context and request, response, etc.

👌 Good to know!

It just feels like it would be nice to support both but perhaps this just makes the whole thing more convoluted. Curious to know your thoughts.

Minimizing the property footprint is a pretty effective way to maximize the leverage of intellisense I feel.

Users could do the following fwiw but I don't see much value in it in practice (in the sense that, I don't think many users will actually want to do this).

(_root, args, { response: res, request: req }) => {
  // ...
}

[jasonkuhrt]

Had a longish discussion with @Weakky about this and #509. It involved things like:

• context is a graphql concept
• but the request lifecycle that we need context scoping for is a server concept
• how to model this in the framework API
• framework level server middleware
• we don't want server.custom to become the thing used in most cases
• we probably don't want addToContext to become an unofficial way to write server middleware! (e.g. write auth logic, throw errors)
• be careful about making users re-learn everything
• the express middleware system is pretty good, koa is neat, anything else?
• yes req/res should probably be exposed on the ctx by default––but what exact view of those is unclear still

We'll continue to mull. Needs a bit more attention to get this right. The framework server abstraction is a bit too weak right now to fully address the issue. Meaning we may need to iterate on what the framework's server system is generally. Then return to this issue concretely.

[robin-macpherson]

@jasonkuhrt Good to know! That makes sense to me regarding maximizing leverage of intellisense, then.

Thanks for sharing those notes from the discussion with @Weakky . I agree that addToContext shouldn't become an unofficial way of writing server middleware, with it is starting to feel like in the current state for me. As a developer using the framework every day now, I feel myself wanting to just know what is "the way" to add my custom server configuration, if I should need to do that at all, and what is "the way" to add my middleware, and again, what do I actually need to implement myself.

I definitely appreciate being a part of the discussion around this 😊

[jasonkuhrt]

I feel myself wanting to just know what is "the way" to add my custom server configuration, if I should need to do that at all, and what is "the way" to add my middleware, and again, what do I actually need to implement myself.

I think its fair to say that @Weakky and I have directionally aligned that Nexus will ship with a server middleware system. A dedicated issue for this will be created soon.