[RFC] Introduce express middleware extension point

[raymondfeng]

This PR introduces an extension point for express middleware so that they can be added and sorted by phase to force a hierarchical router.

Doc: https://github.com/strongloop/loopback-next/blob/express-middleware/packages/express-middleware/README.md

See also #1293

Checklist

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated
• Agree to the CLA (Contributor License Agreement) by clicking and signing

[bajtos]

I am not sure if it's a good idea to re-introduce middleware phases in LB4. From the comments I seen in the past, many LB3 users are not "getting it", they don't understand why should they use middleware phases instead of vanilla express style of registration.

Do we still want to purpose Sequence as the tool for orchestrating request handling pipeline? I think we should focus more on Sequence and treat compatibility with Express middleware as a short-term workaround only.

As I see it, there are principally four middleware phases:

1. request pre-processing
2. route handlers
3. static assets
4. error handler

In LoopBack 3, the primary motivation for introducing middleware phases was to ensure correct order of invocation of these different middleware types. IIRC, the concept of finer-grained phases for request pre-processing was a kind of a nice side effect.

In LoopBack 4, there is already infrastructure in place to enforce these four phases. What we are missing is API that would allow app developers to register their own request pre-processing middleware.

As I see understand the proposal in your PR, it is splitting the request pre-processing phase into multiple finer-grained sub-phases. While I see how that can be useful, I am not convinced that the benefits justify the additional complexity (both for our users and the framework codebase).

[raymondfeng]

In fact, I would like to refactor this PR into a separate module such as @loopback/express to achieve my objectives:

1. Make it easier to leverage LoopBack 4 core (especially the IoC/DI) for existing Express users
2. Make it more scalable for Express applications that consists of multiple modules

• Discover/register middleware (forcing components to use app.use is not feasible)
• Use IoC/DI for middleware or route handlers

3. Incrementally bring in the full REST experience into Express

It's my intent to create an Express++ experience using TypeScript and LoopBack 4 core modules.

For LoopBack itself, I personally don't think the sequence of actions is good enough to take over the Express middleware pipeline and we should keep the door open for developers to use existing middleware.

[raymondfeng]

I refactored the code into @loopback/express-middleware. It's fully functional with documentation. Please review.

[raymondfeng]

I would like to port error and final support from #1671.

[bajtos]

This patch is adding a new package. Have you followed all steps in our
checklist? DEVELOPING.md#adding-a-new-package

How do you envision error handling for errors reported from Express middleware? At the moment, LB4 pretty much guarantees that all errors will be routed to reject sequence action. IIUC, your proposal is changing that - some errors are handled by reject, errors from middleware is handled by the non-customizable final error handler in the RestServer.

On a similar topic: at the moment, it's easy to observe when request handling started and finished, because everything happens inside sequence. For example, to log duration of a request (including failed requests!), one can write similar sequence:

export class MySequence implements SequenceHandler {
  // constructor with @inject decorators

  async handle(context: RequestContext) {
    const {request, response} = context;
    const start = process.hrtime();
    try {
      const route = this.findRoute(request);
      const args = await this.parseParams(request, route);
      const result = await this.invoke(route, args);
      this.send(response, result);
    } catch (error) {
      this.reject(context, error);
    }
    const delta = process.hrtime(start);
    console.log(`Handled "${request.method} ${request.url}" in ${delta[0]} seconds.`);
  }
}

This won't be possible with your new extension in place.

IMO, we should move middleware invocation into a new sequence action.

export class MySequence implements SequenceHandler {
  // constructor with @inject decorators

  async handle(context: RequestContext) {
    const {request, response} = context;
    const start = process.hrtime();
    try {
      this.executeMiddleware(); 
      const route = this.findRoute(request);
      const args = await this.parseParams(request, route);
      const result = await this.invoke(route, args);
      this.send(response, result);
    } catch (error) {
      this.reject(context, error);
    }
    const delta = process.hrtime(start);
    console.log(`Handled "${request.method} ${request.url}" in ${delta[0]} seconds.`);
  }
}

Anyhow. I think the most important task now is to find a different way how to extend @loopback/rest with support for custom middleware, an extension point that will allow LB4 developers to experiment with different approaches for middleware registration.

[bajtos]

Do you expect users to call middleware with a single argument only? If not, then I think the following API may be more ergonomic:

expressCtx.middleware({
  handler: cors(),
  phase: 'cors',
  // path, etc.
});

[bajtos]

I am not comfortable forcing Express context on all LoopBack4 users.

Can you find a way how to make this extension an optional addition please?

[bajtos]

I think that by now, our REST layer should be providing enough features to allow us to implement _serverOpenApiSpec routes as regular LB4 routes with x-visibility: undocumented. Instead of relying on your new extension, can we rework this part to use existing LB4 means?

[bajtos]

Ah, it looks like it was an intentional decision to implement these routes at REST API layer, bypassing the sequence.

https://github.com/strongloop/loopback-next/blob/c3d800700b253e97316fd0871641ea80fcb457f3/packages/rest/src/rest.server.ts#L265-L271

[bajtos]

Uh oh, this is even more complex than I thought. It turns out that OpenAPI endpoints like /openapi.json do not honor basePath configuration set at server level.

[bajtos]

You can see the RouteEntry-based implementation of /openapi.json here: 2b5be22

[bajtos]

Uh oh, this is even more complex than I thought. It turns out that OpenAPI endpoints like /openapi.json do not honor basePath configuration set at server level.

This problem is already tracked here: #2329

[bajtos]

Similarly here. We don't support dynamic redirects yet - see #2022 and the work in progress in #2529, but I think it should be possible to implement "redirecto to Swagger UI" by implementing a LB4 route that's calling response.redirect directly.

[bajtos]

I think this change should be deferred until #2529 and #2547 are landed, at which point we should rework these redirects to leverage server.redirect APIs instead of calling response.redirect directly.

[raymondfeng]

Close to favor #5118