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

Middleware/ext support #34

Open
luvies opened this issue Jun 14, 2020 · 4 comments
Open

Middleware/ext support #34

luvies opened this issue Jun 14, 2020 · 4 comments
Labels
enhancement New feature or request

Comments

@luvies
Copy link

luvies commented Jun 14, 2020

It would be good to support some form of middleware to allow processing of requests/responses in a more general fashion. Hapi uses the following (with some extra overloads):

type ExtPoint = "onPreStart" | "onPostStart" | "onPreStop" | "onPostStop"
  | "onRequest" | "onPreAuth" | "onCredentials" | "onPostAuth" | "onPreHandler"
  | "onPostHandler" | "onPreResponse" | "onPostResponse";

interface ExtOptions {
  before?: string | string[];
  after?: string | string[];
  bind?: object;
  sandbox?: string;
  timeout?: number;
}

interface ExtEvent {
  type: ExtPoint;
  method(request: Request, h: Toolkit): void;
  options?: ExtOptions
}

interface Server {
  ext(events: ExtEvent | ExtEvent[]): void;
  ext(
    event: ExtPoint,
    method?: (request: Request, h: Toolkit) => void,
    options?: ExtOptions,
  ): void;
}

A suggestion I had was to use a modified version, which uses class instances for each request. That way you can keep track of information during a request (such as timing), and allows for an API like so:

interface ExtHandler {
  onPreStart?(request: Request): void;
  onPostStart?(request: Request): void;
  onPreStop?(request: Request): void;
  onPostStop?(request: Request): void;
  onRequest?(request: Request): void;
  onPreAuth?(request: Request): void;
  onCredentials?(request: Request): void;
  onPostAuth?(request: Request): void;
  onPreHandler?(request: Request): void;
  onPostHandler?(request: Request, response: Response): void;
  onPreResponse?(request: Request, response: Response): void;
  onPostResponse?(request: Request, response: Response): void;
}

interface Server {
  ext(handler: new (h: Toolkit) => ExtHandler): void;
}

It should be possible to support both, but that would complicate things possibly. Another option would be to support

interface Server {
  ext(handler: ExtHandler): void;
}

in some way to allow for the case where you don't need to store instance data (if this was supported, each method would also get h as well).

@sholladay
Copy link
Owner

sholladay commented Jun 14, 2020

I believe I understand. In your Server interface, new means passing the class itself, right?

Can you show a usage example with your proposed API to clarify how it would work from the user's perspective? It's like the code below and it would be instantiated for each request, right?

class Middleware {
    constructor(h) {
        this.foo = 'could store some config here'
    }
    onPreHandler(request) {
        this.time = 'could set up a timer here to measure how long the handler takes to execute'
    }
    onPostHandler(request) {
        this.time = 'could stop a timer here and measure how long the handler took to execute'
    }
    // ...
}

pogo.server({
    ext(Middleware)
});

The above is a little too object-oriented for my liking. But I think I see the appeal.

The way that hapi lets you keep track of state like timings is with server.app and request.app. It's just a namespace whose value is an empty object and you can assign whatever you want to it. Kind of makes sense to attach request-level state to the request instance itself, for example. It can be a little tricky to debug, though, since it's not obvious where the state came from.

@luvies
Copy link
Author

luvies commented Jun 15, 2020

That example was pretty much what I was thinking, but whether it's worth implementing is mostly down to how well it fits. Using an API like Hapi's ext might be better for compatibility, so it would be worth implementing that first probably.

For server.app and request.app, it might be worth adding generic parameters so that those properties can be fully typed (improving compile-time safety).

@sholladay sholladay added the enhancement New feature or request label Jun 30, 2020
@fangmarks
Copy link

fangmarks commented Sep 16, 2022

Middleware support would be really great, It'd be really nice if I could log requests I've gotten to either a file or a db

@sholladay
Copy link
Owner

@himbolion agreed, that's one of the top use cases here.

The first step is to implement an event system so you can do this:

server.on('request', (request) => {
    log(request);
});

Then the second step is to implement server.register() so that you can easily package that into a plugin that uses the event system:

const myPlugin = (server) => {
    server.on('request', (request) => {
        log(request);
    });
};

server.register(myPlugin);

Plugins are just functions that are awaited and called with server as the first argument and maybe an options object.

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

No branches or pull requests

3 participants