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

Allow more config options for ServerConfig, or allow passing our own Koa instance. #94

Closed
ADRFranklin opened this issue Nov 9, 2023 · 4 comments · Fixed by #88
Closed

Comments

@ADRFranklin
Copy link

Hi there,

I've been looking for a way to generate a typescript server from an openapi schema that wasn't express based for a while now, and I am happy I found this tool, as it supports both Koa and Zod which are two library I have been using heavily in my projects, so big thanks for making it.

Now I am working on a project where I need to specify additional options to the Koa instance, one being the host options and the ability to use a custom function for the listen callback. Both of these are pretty important requirements for my project, due to how we want our output being presented to the console. Because my project makes use of node clusters, calling the startServer function means that every web cluster instance is now outputting the line server listening: ... which is not ideal, and there is no options to prevent this.

So having the option to override the default as a field in the ServerConfig type would be a good compromise, unless of course you have a better solution in mind.

Thanks

@mnahkies
Copy link
Owner

mnahkies commented Nov 9, 2023

As it would happen I've been experimenting with ways to improve this over in #88

(I've also been a bit lazy and let that PR grow in scope - I'll probably chop it up before merging)

The first commit is the main one relevant to your needs: d181bd8

It separates the construction of the router and starting of the server - this pretty much leaves you free to construct your Koa instance as you see fit, with the caveat that it relies on a body parsing middleware being mounted.

Eg: your entrypoint could look like:

async function main(){
  const app = new Koa()
  app.use(koaBody())

  const router = createRouter({...})

  app.use(router.allowedMethods())
  app.use(router.routes())

  ...do whatever you like with `app`
}

It also removes that console.log you mentioned (01367a7)

Other things happening in that PR, that I'll probably split out to separate changes:

  • Wrapping the raw Zod errors to indicate what part of the request caused these (bb056e9)
  • Introducing a "responder" parameter (b83d362)

Motivation for the wrapped errors is to support error handling middleware like:

export async function errorMiddleware(ctx: Context, next: Next) {
    try {
        await next()
    } catch (err) {
        if (
            KoaRuntimeError.isKoaError(err) &&
            err.phase === "request_validation"
        ) {
            ctx.status = 400
            ctx.body = {
                message: "request validation failed",
                code: 400,
                meta:
                    err.cause instanceof ZodError
                        ? { issues: err.cause.issues }
                        : {},
            } satisfies t_Error
            return
        }

        ctx.status = 500
        ctx.body = {
            message: "internal server error",
            code: 500,
        } satisfies t_Error
    }
}

Eg: treat request validation as a client error, and other errors as internal server errors. You'd probably have your own client errors in the mix as well, but something like this feels like a good baseline to me.

I'm not certain if I'm happy with the responder pattern change yet, and would be interested in your thoughts if you have any. After that change a request handler looks something like:

export const getTodoLists: GetTodoLists = async ({ query }, respond) => {
    const pageSize = query.size ?? 10

    const lists = await prisma.todoList.findMany({
        take: pageSize,
        skip: query.cursor ? 1 : 0,
        cursor: query.cursor ? { id: query.cursor } : undefined,
        orderBy: { created_at: "desc" },
    })

    return respond.with200.body({
        items: lists.map(dbListToApiList),
        cursor: lists[lists.length - 1]?.id ?? null,
        pageSize,
    })
}

Where respond has typed properties based on the openapi spec. My motivation for this was to try and get better intellisense / autocomplete in my editor, and it feels a bit terser as well.

I didn't know this had any users yet, so any user feedback is useful!

@ADRFranklin
Copy link
Author

ADRFranklin commented Nov 9, 2023

I am happy to hear you already had thought about this and even have something in the works, that's pretty good news. So far what you put seems like something I would be happy with.

That being said, I do have some questions about that Error Handling middleware you presented there, is that going to be optional or configurable? I ask because in my project I follow DDD/Hexagonal architecture, so I have my own app error handling with my own structures, and with being the case, the output to the user doesn't contain all the fields I would like to have. So of course being able to provide my own even if mostly based on your current one, with some minor changes to how the output looks would definitely fit my needs more.

The response part is pretty different to what I have seen other projects do, so I am interested in how that would work, based on how it looks though, would there be .body and a .header as well? Idk if maybe something like would make more sense or there are reasons for doing it that way

response.with(200, {
   body: {},
   headers: {}.
});

One other thing while not on topic for this specific issue, but I thought I might mention, is the ability to have the openapi.yaml or openapi.json file byte encoded into the generated files, so that it could be served at something like /api/openapi.json, this could be something enabled with a flag such as --embedSpec or something like that.

Appreciate the quick reply:)

mnahkies added a commit that referenced this issue Nov 11, 2023
- split out a generated `createRouter` function that only creates
  the koa router, making use of the `bootstrap` / `startServer`
  functions optional

- allow passing options to the `cors` / `koa-body` middlewares, or
  disabling these completely

- support passing a `ListenOptions` instead of `port` number, allowing
  to bind to a specific network interface

- return the koa `app` and the `AddressInfo` from `startServer` /
  `bootstrap`

- make `startServer` an `async` function that rejects if `app.listen`
  fails

**this is a breaking change**, and probably not the last to this
interface.
whilst the prior iteration was far too inflexible, this doesn't feel
quite right still:
- `startServer` still pretty inflexible/simple, and doesn't support all
options
- but also now flexible enough to shoot yourself in the foot, eg: `body:
"disable"` and no suitable alternative provided
- `bootstrap` function basically redundant now as well, though that may
not remain true so can stay for the moment

closes #94
@mnahkies
Copy link
Owner

I've split up that PR, and merged the changes relevant to your original post in #88 - released in https://github.com/mnahkies/openapi-code-generator/releases/tag/v0.1.1

Example of new usage:

const {server, address} = await bootstrap({
router: createRouter({
findPets: notImplemented,
findPetById,
addPet: notImplemented,
deletePet: notImplemented,
}),
port: {port: 3000, host: "127.0.0.1"},
})

The other two changes I mentioned are now WIP in these PR's:
#95 - feat!: throw discriminable errors indicating point of failure
#96 - feat!: responder pattern

Feel free to take a look and provide feedback - the error handling middleware is very much optional and BYO, the motivation of the change is to provide additional context on the thrown errors to enable creating such middleware, but I don't think it's possible to generate a meaningful global error handler without making too many assumptions about the API spec in the generator.

Still working out the details for the responder pattern one, response headers are something that's still on the TODO list.

For the surfacing of the openapi.yaml spec, I do have #81 for adding support for redoc which is kinda similar. I'm still working out what the boundary/scope of the project should be for stuff like this though. I like redoc, but many people prefer swagger-ui - how should a library like this avoid forcing too many choices like that on people.

@ADRFranklin
Copy link
Author

Awesome, I will be giving the new version ago soon, very happy to see this change. After looking at the PR for the responder type, that made it much easier to understand what you were going for, and this seems perfectly usable to me, even more usable once header support makes it.

While supporting stuff like swagger-ui and redoc sounds interesting, though what I was asking for was simply allowing to serve the spec file as a resource, so other projects could just generate the client from that spec, and you know the spec served will work for that specific version. There are cases where someone is running an outdated version of project for specific reasons and it would be a problem if the project itself only hosted the up to date version of the spec, so generating it for older versions would cause incompatibilities, so that would be my main reason for having this feature.

I appreciate all the work going into this, I have high hopes for this library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants