feat(rest): enable Express middleware to be used as interceptors with LoopBack #5118

raymondfeng · 2020-04-16T04:18:31Z

Depends on #5317
Implements #2035
Supersedes #2434

This PR adds ability to use Express middleware with LoopBack 4 as follows:

The default sequence now has Middleware step. It creates an invocation chain to call registered middleware handlers via extension point/extension pattern.
The sequence can be customized to have more than one Middleware steps.
Express middleware can also be wrapped as LB4 interceptors, which can in turn be added to global/class/method level.
Move built-in cors and openapi endpoints to middleware action.

restServer -> sequence (middleware, findRoute, ..., invoke, send) --> global middleware interceptor --> class/method level interceptor.

Some of the ideas are inspired by https://github.com/codejamninja/lb4-middleware. Thanks @codejamninja and @victor-enogwe

See docs at:

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

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

👉 Check out how to submit a PR 👈

raymondfeng · 2020-04-16T04:26:18Z

See tests at https://github.com/strongloop/loopback-next/tree/middleware-interceptor/packages/rest/src/__tests__/acceptance/middleware.

docs/site/Express-middleware.md

hacksparrow · 2020-04-16T14:24:43Z

docs/site/Express-middleware.md

+We create a few helper functions that wraps an Express middleware module into an
+LoopBack 4 interceptor.
+
+### Create an interceptor from Express middleware module name or factory function


Since this file will be a reference doc, we should prioritize on user API documentation. Including implementation details should be an added bonus, we could even omit it, IMO.

We can improve it once the apidocs is live.

bajtos

Great initiative! I appreciate that your new design is addressing the concerns I described in #2434 (comment) and #2434 (review).

I don't have bandwidth to review this proposal in full, so I am posting just few comments on things that caught my eye when skimming through the docs.

docs/site/Express-middleware.md

packages/rest/package.json

bajtos · 2020-04-16T14:58:09Z

packages/rest/src/middleware/middleware-interceptor.ts

+): GenericInterceptor<CTX> {
+  let factory: MiddlewareFactory<CFG>;
+  if (typeof middleware === 'string') {
+    factory = require(middleware);


Please note this require call will resolve the path relatively to the location of @loopback/rest module, not relatively to the code calling createInterceptor. This creates confusing behavior when createInterceptor is called e.g. from a 3rd-party package that's symlinked into node_modules tree.

We have been bitten by this problem multiple times in the past. While there are ways how to work around it, I feel the complexity is not worth the benefits.

Can you please remove the string-based variant and always require the factory function instead?

Also what if the middleware does not provide any factory function and all the users have is a regular function (req, res, next) => void? Are they supposed to wrap that function into a factory just for the sake of satisfying this API?

Last but not least, how do you envision caching the instances of middleware between requests? It's important to call middleware factory function only once at app startup, we must not call it for every incoming request. I don't yet fully comprehend how is createInterceptor going to be used, so maybe this is not a problem? IDK.

I also feel this function is violating Single Responsibility Principle, because it has two responsibilities:

Obtain an instance of middleware handler.

Convert a middleware handler to an interceptor.

Have you considering removing the first responsibility and modifying the function to accept the middleware handler only?

Support for middleware module as string is removed.

Default InvokeMiddleware is set to no-op.

Caching is handled at binding scope level.

bajtos · 2020-04-16T15:01:04Z

@raymondfeng this is a sizeable change with major design implications, can you please work with @dhmlau to create space in our plan to give us all enough time to review the proposal?

deepakrkris

LGTM

deepakrkris · 2020-04-30T15:37:31Z

packages/rest/src/sequence.ts

@@ -95,6 +103,8 @@ export class DefaultSequence implements SequenceHandler {
  async handle(context: RequestContext): Promise<void> {
    try {
      const {request, response} = context;
+      // Invoke registered Express middleware
+      await this.invokeMiddleware(context);


Can we have interceptors for each step in the default sequence, like before and after for find route, parse params, invoke, etc, similar to how we had phases in loopback 3 ( initial -> session -> auth -> parse -> routes -> files -> final ) . So, users dont have to write their own sequence class every time just for adding an authentication step.
The default sequence then becomes more reusable, than just being a template.

deepakrkris · 2020-04-30T15:44:05Z

packages/express/src/express.server.ts

+/**
+ * An Express server that provides middleware composition and injection
+ */
+export class ExpressServer extends MiddlewareRegistry implements Server {


It is more intuitive to think ExpressServer as an extension of RestServer and a mixin of MiddlewareRegistry. Also then we could change any existing LB4 app as an express app without the need for composition with multiple apps. This could become a great advantage for having LB4 web apps, and that opens a lot of use cases for LB4.

We could make MiddlewareRegistry a mixin if needed.

deepakrkris · 2020-04-30T15:49:06Z

packages/express/src/express.application.ts

+/**
+ * A LoopBack application with Express server
+ */
+export class ExpressApplication extends Application {


+1, this makes sense extending ExpressApplication from Application, similarly can we extend ExpressServer from RestServer, so that any existing LB4 app can become an express app.

deepakrkris · 2020-04-30T16:07:56Z

packages/express/src/__tests__/acceptance/test-helpers.ts

+      }
+    }
+    const binding = this.app.controller(MyController);
+    this.app.expressServer.expressApp.post('/hello', async (req, res, next) => {


Any advantage we have, by this being an independent package ?

…ware - add `Middleware` as interceptors that use request context - add `InvokeMiddleware` to invoke middleware in a chain - add extension points for Middleware extensions - add helper methods to adapt express middleware as interceptors - add helper methods to adapt LoopBack middleware chain as Express middleware - add ExpressServer and ExpressApplication to extend Express with LoopBack core - allow dynamic configuration changes for Express middleware - allow `toInterceptor` to take one or more Express middleware handlers - add MiddlewareMixin - make namespaces consistent and simplify chain invocations

- add `InvokeMiddleware` action to the default sequence - add `middleware/expressMiddleware` APIs to RestServer/RestApplication - use middleware APIs to register CORS and OpenAPI spec endpoints BREAKING CHANGE: `basePath` now also applies to endpoints that serve OpenAPI specs. For example, the OpenAPI specs are available at `/api/openapi.json` or `/api/openapi.yaml` if the base path is set to `/api`.

…/rest

jannyHou

👍 Awesome! LGTM.

raymondfeng requested review from deepakrkris, emonddr and hacksparrow as code owners April 16, 2020 04:18

raymondfeng mentioned this pull request Apr 16, 2020

[RFC] Introduce express middleware extension point #2434

Closed

8 tasks

raymondfeng self-assigned this Apr 16, 2020

raymondfeng added the REST Issues related to @loopback/rest package and REST transport in general label Apr 16, 2020

raymondfeng force-pushed the middleware-interceptor branch from 009cbc0 to a5911ca Compare April 16, 2020 05:28

raymondfeng requested review from agnes512, bajtos, jannyHou and nabdelgadir as code owners April 16, 2020 05:28