feat: add server.custom api #382

Weakky · 2020-02-10T19:22:17Z

Description

Add a server.custom api to provide custom server
This API can also be used to access the underlying express instance

Usage examples:

Using fastify

import { settings, server } from 'nexus-future'
import Fastify from 'fastify'
import GQL from 'fastify-gql'

server.custom(e => {
  const app = Fastify()

  return {
    async start() {
      app.register(GQL, {
        schema: e.schema,
        context: req => e.createContext(req)
        ide: 'graphiql',
      })

      await app.listen(e.settings.port)

      console.log(
        `Server listening at http://localhost:${e.settings.port}/graphiql`
      )
    },
    stop() {
      return app.close()
    },
  }
})

Accessing the default express instance to do some stuff

server.custom((e) => {
  e.defaultServer.instance.use(/*some custom middleware*/)

  return e.defaultServer
})

TODO

docs
tests

jasonkuhrt

Excited to get this out the door! Found it hard to write my feedback for the API, its tricky! No blocking comments!

src/framework/app.ts

src/framework/schema/config.ts

src/framework/schema/index.ts

src/framework/server.ts

jasonkuhrt · 2020-02-11T04:58:34Z

src/framework/server.ts

+
+interface CustomServerHookInput {
+  schema: GraphQL.GraphQLSchema
+  defaultServer: ServerWithInstance<Express>


Mutually exclusive comments:

👏

One thing, wonder if we should go with originalServer. Thinking of how I played with this on the Settings API. But we never talked about it! Maybe the Settings API should change to be settings.defaults instead.

But either way, I think probably using consistent terminology is the most important thing.

I do kind of like softening the terminology a bit where we can to make it a little more humane/literate. For example settings instead of config.

Thoughts?

Since we're strongly typing this as Express what's the point of this not just being an Express reference called express?

I guess plugins could one day supply custom server and then user wants to to tap into it just for side-effects (e.g. add middleware).

Then, I see value, because then, plugins could turn defaultServer from Express to e.g. Fastify.

This was indeed the original goal. But your comment made me realize that nothing was ready for plugins to override the type anyway so it might be a bit premature. I reverted and "hardcoded" express into its own type for now!

Regarding original vs default, I think original is probably better. That's usually how people name their variable when overriding functions. FWIW, I have an example in mind, taken from the TS compiler API documentation

Notice how both variables are called orig<something>, where orig is most likely referring to original.

TLDR: I renamed to property to originalServer 👍

jasonkuhrt · 2020-02-11T05:07:23Z

src/framework/server.ts

+interface CustomServerHookInput {
+  schema: GraphQL.GraphQLSchema
+  defaultServer: ServerWithInstance<Express>
+  settings: App.SettingsData['server']


Calling this serverSettings would be more consistent with other name of defaultServer?

Renamed too! I agree with you, I was initially passing the whole settings object. If I may be really neat-picking here, I don't like the fact that we have <server>Settings and default<Server> (server is used as prefix and as suffix).
Makes me wonder if it'd be better to scope everything under a single server property.

server: { original: Express settings: SettingsData['server'] }

Either server.originalInstance or server.original. That'd be even more consistent with settings.original

Ah, just saw your comment below. Yeah, you're right, let's just go with express and let them take the settings the import

jasonkuhrt · 2020-02-11T05:31:29Z

src/framework/server.ts

+  instance: T
+}
+
+interface CustomServerHookInput {


This feedback invalidates some of the below. Actually a bit tricky of an API/naming here, so pardon the multiple takes I'm giving...

I don't think we need to pass settings reference because user can get it from lexical scope.

I think it might be simplest to just do:

server.custom(({ express }) => { }

something else, not sure...

// simple case server.custom(express => { express.use(...) return { express.listen(settings.current.server.port) } })

// advanced case server.custom(express => { express.use(...) return { start(){ // user can lexically access settings, should be fine! express.listen(settings.current.server.port) } } })

// most advanced case server.custom((_, { schema }) => { // User can get at schema, may ignore originalServer instance if they wish // I don't think we actually need the `settings` here, should be // good enough with lexical scope access. })

or...

// most advanced case server.replace(({ schema }) => { // User can get at schema, may ignore originalServer instance if they wish // I don't think we actually need the `settings` here, should be // good enough with lexical scope access. })

It supports (by way of simplest api/syntax) a primary use-case of configuring express, not swapping the server completely.

It allows user to name the server as it actually is

.replace can improve boot performance because we can skip creating express instance completely...

Then we need to make it a runtime error if user calls both replace and custom...

Basically back to raw 🙈

Actually, I'm reconsidering your suggestion. I'm fine with calling it express, but I think I'd prefer keeping express of type Server and not the raw Express. It makes the usage way simpler in case you just want to add middlewares and other kind of stuff to the express instance. All you need to do is mutate the express (previously originalServer) property, and return it.

As to your suggestion for replace/custom, I personally find it easier and more generic the way it is now. There's no need to wonder what's the difference between replace and custom, no possibility of errors if you actually use both, and, with my suggestion just above, it makes it very simple to just configure express

server.custom(({ originalServer }) => { originalServer.instance.use(/*some custom middleware*/) return originalServer })

Even cleaner pattern IMO if you have too much express related stuff:

import { customizeExpress } from './custom-express' server.custom(({ originalServer }) => { return customizeExpress(originalServer.instance) })

How about we make it so returning is optional and if you don’t it means do not override the server? I think this is most clear. It does create more room for user error though. If we had replace and custom then we could make more precise return type. But not sure it’s worth the extra dpi surface. Think the return-to-replace is the sweet spot.

I'd prefer keeping express of type Server and not the raw Express. It makes the usage way simpler in case you just want to add middlewares and other kind of stuff to the express instance.

Then user should never need access to server interface. Simplest of all?

jasonkuhrt · 2020-02-11T05:33:37Z

Hey @nhuesmann, your 0.02 on this if any would be welcome.

nhuesmann · 2020-02-11T11:41:53Z

Hey @jasonkuhrt I just saw your mention. I took a look at this, so far I like where it's headed. To clarify:

The custom method needs to return an object with start and stop methods. It can be any GraphQL server library, as long as it satisfies that- correct?
it would appear that the underlying defaultServer has both a start and stop method, and that is why it's being returned after adding middleware- is that correct? That was the only part I found slightly confusing- since the express app is a Singleton, I'm used to just modifying it directly through server.use, so I wasn't expecting it to need to be returned after modifying it in the example where you add middleware to defaultServer. However if it's done in order to meet the criteria of the return object interface with a start and stop method, then I get it and it feels fine.

So far this feels good!

Weakky · 2020-02-11T13:03:59Z

Hey @nhuesmann,

Yeah, this is an attempt at reconciling the raw api and the "custom server" api. Instead of providing two different apis, we thought we could let people customize the default express instance by using the custom server api which is injected with the express instance.

The idea of the raw api where you could access the instance via server.raw.<anything> is confusing alongside the server.custom api, because the two features overlap.
Once you'd use the server.custom api, it's tempting to wonder where you're supposed to customize your server. Directly in the server.custom callback, or in the server.raw property ? It also makes sure that people cannot do crazy stuff on their express instance anywhere in their project, which might not be evaluated at the right time.

I'm not sure my explanation is clear, sorry if it's not 🙈

Weakky · 2020-02-11T13:12:36Z

Note: I added a missing feature to pass the nexus generated context to a custom server. The server.custom callback now provides a createContext(req) => Context function to be used.

I updated the example at the top to reflect the changes.

EDIT: @jasonkuhrt Made me think about something though: That feature is error-prone. It's very tempting to do:

server.custom(({ schema, createContext }) => {
  const customServer = new GraphQLServer({
    schema,
    context: req => ({
      ...createContext(req),
      additionalProperty: 'foo' // <== Wrong usage. If people do that, they won't have type-safety in their resolvers because the only way should be through `schema.addToContext()`
    })
  })
})

If people add any additional property "manually", they won't have type-safety in their resolvers because the only way should be through schema.addToContext().

Also makes me wonder if addToContext should live under schema or server. While I understand the reasons why it's currently under schema (I guess because the schema is from where you access the context), I can see reasons why we'd want to move it to the server component, as that's where the context is actually defined.

It also makes it less awkward for the problem above. Having schema.addToContext have effects on the createContext of server.custom is weird and feels disconnected.

EDIT2: Continuing train of thoughts, worth wondering then if it should live under the settings component... 🤦‍♂️

EDIT3: Here's my position after thinking more: server.custom is an escape-hatch-ish. It's not entirely an escape hatch because I don't consider adding middleware to the express instance something that's borderline, but adding a custom server can reasonably be considered an escape hatch.

Under these assumptions, having a slightly error-prone API when dealing with custom stuff doesn't bother me that much, especially because you're not exposed to the context stuff when you're just willing to add middlewares to the express server.

Therefore, I'd say we can properly document the limitation (that context should exclusively be contributed through the addToContext API) and sleep fine at night 🤓💤

I'm still preferring server.addToContext vs schema.addToContext now though!

nhuesmann · 2020-02-11T14:19:42Z

@Weakky I just got caught up.

I think trying to unify editing both a custom server and the default server via server.custom makes sense. The only potential issue I see is that it feels a little awkward to call server.custom then access defaultServer.instance in order to change something on the default server (I.e middleware). I'm wondering if it'd help to change the name from server.custom to server.customize? Especially since this is a method, so using a verb makes sense, and it feels like it semantically makes more sense that I could call customize to customize/change either the default server or supplement my own server. I dono, just thinking out loud, take it or leave it.
Is there a reason for calling e.defaultServer.intance.use? Is that much nesting necessary, or could you get away with e.defaultServer.use? Again, just thinking out loud.
Will ALL the customization/setup for a custom server (I.e fastify-gql) take place within a single server.custom call?
As for schema.addToContext vs server.addToContext, I think I'm ok with either. Are there any other user-callable methods on schema? If not, I'd say just move addToContext to server. It might feel better there anyway 🤷🏼‍♂️

Anyway, that's what I've got for now, hope that helps. Lemme know if any of it needs clarification.

jasonkuhrt · 2020-02-11T15:39:25Z

@nhuesmann thanks for the input! Really nice to have another angle in the discussion 🙏

Sketches from live share session

import Fastify from 'fastify'
import GQL from 'fastify-gql'
import { server } from 'nexus-future'

type Server = any
type ServerLens = any

type ServerReplacer = (lens: ServerLens) => Server
type ServerAugmenter = (lens: ServerLens) => void

// server.custom
// server.customize
// server.tap 

// augment

// server.custom(({ express }) => {
//   express.use(...)
//   // no return, does not replace
// })

// // ???
// server.customize((express, { schema }) => {
//   expreess.use() // 
// })

// // ???
// server.tap((express, { schema }) => {
//   expreess.use() // 
// })

// // ???
// server.customize(({ express, schema }) => {
//   express.use()  
// })



// replace pattern
server.custom(({ express }) => {
  // inside server
  // ...
  // -->

  express.use(...)

  // return to outside server
  // ... 
  // <-- 

  return {
    start() {},
    stop() {}
  }
})


// tap pattern
server.custom(({ express }) => {
  express.use(...)
})



const app: any
const schema: any
const settings: any




// replace

server.custom(({ context, schema }) => {
  const app = Fastify()

  return {
    async start() {
      app.register(GQL, {
        schema,
        context,
        ide: 'graphiql',
      })

      await app.listen(settings.current.server.port)

      console.log(
        `Server listening at http://localhost:${e.settings.port}/graphiql`
      )
    },
    stop() {
      return app.close()
    },
  }
})

// the new addTocontext

schema.context(req => {})
schema.context<Request>(req => {})

app.schema
app.server


server.start
server.stop
server.addToSchemaContext(req => {})
server.addToContext
server.addContext
server.context

server.custom // ize

schema.context(req => {

}) // 

// server.addContext({
//   // ...
// })

// schema.addContext({
//   // ..
// })

// schema.addContext(() => {

// })

// add to context ????
// integration concern



// future no-magic api
// aka. explicit integration

// server.on('request', (req) => {
//   schema.addContext({
//     user: {
//       id: req.id
//     }
//     // ...
//   })
// })

Summary:

Give custom instead of customize a try
Use custom return type to infer replace vs augment semantic 🤞
Add framework debug-level logs about if server is being replaced or augmented
Simplify ServerLens type to have just express property
Simplify ServerLens type to have just context property (vs createContext)
Add type variable to addToContext for request type
Do not change addToContext for now (@jasonkuhrt might start a campaign on trying to rename to just context 🐒)

Weakky · 2020-02-11T16:44:47Z

@jasonkuhrt changes implemented on latest commit. I also made it zero-cost at runtime in case a custom server is provided. The implementation is a bit tricky but I think it's safe and fairly nice. Last round of review possible if you want!

Updated usage examples:

Using a custom fastify server

import { settings, server, schema } from 'nexus-future'
import Fastify, { FastifyRequest } from 'fastify'
import GQL from 'fastify-gql'

server.custom(async e => {
  const app = Fastify()
  return {
    async start() {
      app.register(GQL, {
        schema: e.schema,
        context: e.context,
        ide: 'playground',
      })
      await app.listen(settings.current.server.port)
      console.log(
        `Server listening at http://localhost:${settings.current.server.port}/graphiql`
      )
    },
    stop() {
      return app.close()
    },
  }
})

// Output:
// ○ server:Custom server used

Simply accessing express instance

server.custom(e => {
  e.express.use(/* ... */)
})

// Output:
// ○ server:Augmented Express server used

Weakky · 2020-02-11T16:54:54Z

@nhuesmann

The only potential issue I see is that it feels a little awkward to call server.custom then access defaultServer.instance in order to change something on the default server (I.e middleware)

As Jason mentioned above, we went with an api that makes it easier/less awkward to access the express instance. You can see an example on my comment above

Will ALL the customization/setup for a custom server (I.e fastify-gql) take place within a single server.custom call?

Yes. But you can easily just move your code into a separate function to keep things clean. Any reason to be worried about everything being setup there?

jasonkuhrt

Looks great!

Question about the example code, why is the argument named e?

src/framework/server.ts

Weakky · 2020-02-11T17:12:36Z

No particular reason, I initially called it e because the customizer was named hook, and e referred to event. Right now, the name of the param in the type is called lens: CustomizerLens, but I find it a bit hard to understand/intimidating when used

server.custom(lens => {
  lens.express.*
})

I'm currently writing an example for fastify. What would you suggest as naming?

EDIT: Decided not to give it a name and destructure it 😄

nhuesmann · 2020-02-11T17:43:29Z

@Weakky I dig the change to (e/lens).express, it's way cleaner. Also I only asked about everything being defined in a single call to server.custom because I thought that made the most sense, was going to suggest restricting it to a single call to custom (as opposed to allowing modularization). It was just a random thought.

Excited about this progress! Thanks for letting me be involved.

nhuesmann · 2020-02-11T22:02:14Z

@Weakky @jasonkuhrt could you guys explain the thought behind the name lens?

jasonkuhrt · 2020-02-11T22:07:21Z

It’s not particularly deep. The idea there being a kind of mini API/handle into a part/this part of the system. Nothing to do with Functional programming lenses... heh. What would you call this kind of thing?

_{Sent with GitHawk}

jasonkuhrt · 2020-02-11T22:11:27Z

@nhuesmann agreed about multiple calls ambiguity. In a sense this arguably is more like a setting dressed up as an API. I cannot think of a good reason there would be multiple calls to custom for server replacement. I kind of could see, more, multiple calls for adding middleware if for some reason that was spread through the codebase. But seems exotic. It’s a quirk for sure, but, somehow don’t mind it right now.

_{Sent with GitHawk}

nhuesmann · 2020-02-11T22:19:58Z

@jasonkuhrt Re: multiple calls, cool, glad you think so. Re: lens- Gotcha. I think I just brought it up bc Prisma just renamed Photon/Lift for greater clarity, and lens just didn't immediately click for me. I'm just one person though, so I'd get feedback from more people before making any changes. Also Photon/Lift were public APIs whose names were being interacted with constantly - If lens primarily appears as a TS typing I'd say don't sweat it. As far as a reasonable alternative, I'd need to think about it- because lens has been provided my mind can't get around that name right now haha. I'll just ping if something perfect comes to mind.

nhuesmann · 2020-02-11T23:29:27Z

@jasonkuhrt Ideas, though not great: DefaultServer, ServerDefaults, ServerConfig, CustomizerConfig. Whatever you go with, I feel like it'd be easiest to just have the docs/examples lean toward destructuring whatever properties are needed in the customizer function (as @Weakky did in his example).

Last 2 cents- I still feel like the custom method makes more sense as customize. When you call this method, you are changing server by either modifying the underlying express Singleton (express.use) or replacing the default server entirely. Either way, the result of the custom method call is a customized/changed server, so you are custom-izing it. Other places where verbs are used for method names in this codebase that make sense: app.use (Express), server.addToContext (nexus-future).

This isn't critical, I just was reading through the code changes in the PR as well as @Weakky's updated example and it stuck out to me again. Ultimately go with whatever you guys feel best about. Also full disclosure- my degree is in English, so I sometimes fret about variable names and their varying degrees of clarity a wee bit too much 🙃

feat: add server.custom api

11063e2

Weakky force-pushed the feat/custom-server branch from 6ca1a15 to 11063e2 Compare February 10, 2020 19:25

Weakky added 2 commits February 10, 2020 20:33

Merge branch 'master' into feat/custom-server

2bcc353

feat: pass only server settings to server.custom

8ed1d95

jasonkuhrt self-requested a review February 11, 2020 04:22

jasonkuhrt added scope/server Related to the server component type/feat Add a new capability or enhance an existing one labels Feb 11, 2020

jasonkuhrt assigned Weakky Feb 11, 2020

jasonkuhrt approved these changes Feb 11, 2020

View reviewed changes

feat: add createContext and refactor server

f43580f

refactor: remove settings from customizer, update types and add doc

6618a9c

feat: allow customizing express without returning the server

cceb495

rename type

02ead4d

jasonkuhrt approved these changes Feb 11, 2020

View reviewed changes

src/framework/server.ts Show resolved Hide resolved

Weakky merged commit f9a3095 into master Feb 11, 2020

Weakky deleted the feat/custom-server branch February 11, 2020 17:32

Weakky mentioned this pull request Feb 12, 2020

Raw access to underlying server instance #369

Closed

jasonkuhrt mentioned this pull request Feb 17, 2020

Integrated server support for CORS #380

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add server.custom api #382

feat: add server.custom api #382

Weakky commented Feb 10, 2020 •

edited

Loading

jasonkuhrt left a comment

jasonkuhrt Feb 11, 2020

Weakky Feb 11, 2020 •

edited

Loading

jasonkuhrt Feb 11, 2020

Weakky Feb 11, 2020 •

edited

Loading

Weakky Feb 11, 2020

jasonkuhrt Feb 11, 2020

Weakky Feb 11, 2020 •

edited

Loading

jasonkuhrt Feb 11, 2020

jasonkuhrt Feb 11, 2020

jasonkuhrt commented Feb 11, 2020

nhuesmann commented Feb 11, 2020 •

edited

Loading

Weakky commented Feb 11, 2020

Weakky commented Feb 11, 2020 •

edited

Loading

nhuesmann commented Feb 11, 2020

jasonkuhrt commented Feb 11, 2020 •

edited

Loading

Weakky commented Feb 11, 2020 •

edited

Loading

Weakky commented Feb 11, 2020

jasonkuhrt left a comment •

edited

Loading

Weakky commented Feb 11, 2020 •

edited

Loading

nhuesmann commented Feb 11, 2020

nhuesmann commented Feb 11, 2020

jasonkuhrt commented Feb 11, 2020

jasonkuhrt commented Feb 11, 2020 •

edited

Loading

nhuesmann commented Feb 11, 2020

nhuesmann commented Feb 11, 2020

feat: add server.custom api #382

feat: add server.custom api #382

Conversation

Weakky commented Feb 10, 2020 • edited Loading

Description

TODO

jasonkuhrt left a comment

Choose a reason for hiding this comment

jasonkuhrt Feb 11, 2020

Choose a reason for hiding this comment

Weakky Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

jasonkuhrt Feb 11, 2020

Choose a reason for hiding this comment

Weakky Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

Weakky Feb 11, 2020

Choose a reason for hiding this comment

jasonkuhrt Feb 11, 2020

Choose a reason for hiding this comment

Weakky Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

jasonkuhrt Feb 11, 2020

Choose a reason for hiding this comment

jasonkuhrt Feb 11, 2020

Choose a reason for hiding this comment

jasonkuhrt commented Feb 11, 2020

nhuesmann commented Feb 11, 2020 • edited Loading

Weakky commented Feb 11, 2020

Weakky commented Feb 11, 2020 • edited Loading

nhuesmann commented Feb 11, 2020

jasonkuhrt commented Feb 11, 2020 • edited Loading

Weakky commented Feb 11, 2020 • edited Loading

Weakky commented Feb 11, 2020

jasonkuhrt left a comment • edited Loading

Choose a reason for hiding this comment

Weakky commented Feb 11, 2020 • edited Loading

nhuesmann commented Feb 11, 2020

nhuesmann commented Feb 11, 2020

jasonkuhrt commented Feb 11, 2020

jasonkuhrt commented Feb 11, 2020 • edited Loading

nhuesmann commented Feb 11, 2020

nhuesmann commented Feb 11, 2020

Weakky commented Feb 10, 2020 •

edited

Loading

Weakky Feb 11, 2020 •

edited

Loading

Weakky Feb 11, 2020 •

edited

Loading

Weakky Feb 11, 2020 •

edited

Loading

nhuesmann commented Feb 11, 2020 •

edited

Loading

Weakky commented Feb 11, 2020 •

edited

Loading

jasonkuhrt commented Feb 11, 2020 •

edited

Loading

Weakky commented Feb 11, 2020 •

edited

Loading

jasonkuhrt left a comment •

edited

Loading

Weakky commented Feb 11, 2020 •

edited

Loading

jasonkuhrt commented Feb 11, 2020 •

edited

Loading