Improve IRouter typings

[mshustov]

Problem

the current IRouter implementation has following interface:

import { ObjectType } from '@kbn/config-schema';
get: <P extends ObjectType, Q extends ObjectType, B extends ObjectType>(
    route: RouteConfig<P, Q, B>,
    handler: RequestHandler<P, Q, B>
) => void;

We utilize 3 generics here to allow TS to refer types in route.validate and handler. That provides easy-to-start interface when you inline a route handler, at the cost of coupling all interfaces to @kbn/config-schema library

import { ObjectType } from '@kbn/config-schema';
router.get({
  path: '/',
  validate: {
    body: schema.object({
      bar: schema.string(),
    }),
  },
  (context, req, res) => { // body infers types from validate.body
    req.body.bar; // string
  };
});
// or
const validate =  {
  body: schema.object({
    bar: schema.string(),
  }),
};
router.get({
  path: '/',
  validate,
  (context, req, res) => {
    req.body.bar; // string
  };
});
// If you don't inline a route handler you have to specify typings manually:
const validate =  {
  body: schema.object({
    bar: schema.string(),
  }),
};
const handler = (
  context: RequestHandlerContext,
  req: KibanaRequest<unknown, unknown, TypeOf<typeof validate.body>>,
  res: KibanaResponseFactory
) => {
  req.body.bar; // string
};

// or 
const handler: RequestHandler<ObjectType<any>, ObjectType<any>, typeof validate.body> = (context, req, res) => {
  req.body.bar; // string
};

router.get({
  path: '/',
  validate,
  handler,
});

That creates confusion for plugin authors when you want to a wrapper on top of IRouter methods. An example from @jasonrhodes, when route wrapper allows configuring request method. This wrapper has to define all 3 generics for IRouter methods to provide type safety. This requirement is not obvious to plugin authors.

  type MyRouteConfig<
    P extends ObjectType,
    Q extends ObjectType,
    B extends ObjectType
  > = RouteConfig<P, Q, B> & {
    method: RouteMethod;
  };

 function registerRoute<P extends ObjectType, Q extends ObjectType, B extends ObjectType>(
  route: MyRouteConfig<P, Q, B>,
  handler: RequestHandler<P, Q, B>
) {
  const { method, ...options } = route;
  router[method](options, handler);
}

We can minimize necessity in IRouter wrappers if provide an interface where route.validation and handler are part of the same object.

interface NewRouterConfig<P extends ObjectType, Q extends ObjectType, B extends ObjectType> {
  path: string,
  method: 'get' | 'post' | 'put' | 'delete';
  options: Options;
  validate: Schema<P, Q, B>;
  handler: RouteHandler<P, Q, B>;
}
IRouter{
  public add(config: RouterConfig): void
}

But this doesn't solve the problem that plugin authors have to declare generics for custom wrappers

type MyCustomConfig<P extends ObjectType, Q extends ObjectType, B extends ObjectType> = NewRouterConfig<P, Q, B> & { myProperty: string };

We cannot get rid of all generics completely, because we want that TS to infer validation types with route handler types out-of-the-box. Although we can simplify typings if we switch generic interfaces to typescript primitives instead of using @kbn/config-schema.

interface IRouter {
  get: <P extends object = {}, Q extends object = {}, B extends object = {}>(
     route: RouteConfig<P, Q, B>,
     handler: RequestHandler<P, Q, B>
  ) => void;
}

interface RouteConfig<
  P extends object = {},
  Q extends object = {},
  B extends object = {}
>{
  validate(params: unknown, query: unknown, body: unknown)  => {
    params?: P;
    query?: Q;
    body?: B;
  };
}

const validate =  {
  body: schema.object({
    bar: schema.string(),
  }),
};
router.get<{}, {}, TypeOf<typeof validate.body>>({
  path: '/',
  validate: (params: unknown, query: unknown, body: unknown) => {
    return {
      body: validate.body.validate(body, {}, 'request body'),
    };
  },
  handler: (context, req, res) => {
    req.body.bar; // string
  },
});

This change allows a plugin to use any validation library they want or do not implement validation at all. This is the main problem of this approach as well, I'd prefer platform forces good practices, therefore it's a no-go option for us. Want to hear @elastic/kibana-platform opinion. Related discussion: #44170

Possible actions:

• introduce common purpose interface core.http.route instead of IRouter.get, IRouter.post, etc. as proposed in [Discuss] Add NP core.http.route #44174
• document how to register routes with examples from this issue

[elasticmachine]

Pinging @elastic/kibana-platform

[jasonrhodes]

I'd prefer platform forces good practices, therefore it's a no-go option for us

My opinion on this is that this stance is likely to cause you and others more pain than it will be worth in the end, as people will still eventually hack around the limitations but others will be penalized for not being able to easily do things a slightly different way (such as using io-ts, as a very big example I can already think of off the top of my head). Because of that, it seems like that last example would be a great approach.

I think the harder and more complicated it is to do simple things like "create a wrapper for a request handler", the worse the migration effort is going to go, overall. My $0.02. Thanks for writing this up!

[rudolf]

We can minimize necessity in IRouter wrappers if provide an interface where route.validation and handler are part of the same object.

I just noticed we have an existing abstraction in x-pack that creates a router API similar to what's in NP now in favour of the config based routing: #30299

@bmcconaghy Was there a reason a router with methods like router.post was preferred above hapi's server.route which takes a configuration object?

[mshustov]

should be done as a part of #50179