refactor(rest): use dynamic value provider for actions

Signed-off-by: Raymond Feng <[email protected]> BREAKING CHANGE: If you use one of the built-in action providers as the base class, this commit will break you as the signature of the base class has changed. Otherwise the code should be backward compatible for existing applications.
loopbackio · Aug 20, 2020 · fbe8fd2 · fbe8fd2
1 parent de2f5be
commit fbe8fd2
Show file tree

Hide file tree

Showing 12 changed files with 146 additions and 131 deletions.
diff --git a/packages/express/src/providers/invoke-middleware.provider.ts b/packages/express/src/providers/invoke-middleware.provider.ts
@@ -5,11 +5,11 @@
 
 import {
   Binding,
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
   config,
   CoreTags,
   extensionPoint,
   inject,
-  Provider,
 } from '@loopback/core';
 import debugFactory from 'debug';
 import {DEFAULT_MIDDLEWARE_GROUP} from '../keys';
@@ -27,25 +27,26 @@ const debug = debugFactory('loopback:rest:middleware');
  * Extension point for middleware to be run as part of the sequence actions
  */
 @extensionPoint(DEFAULT_MIDDLEWARE_CHAIN)
-export class InvokeMiddlewareProvider implements Provider<InvokeMiddleware> {
-  /**
-   * Inject the binding so that we can access `extensionPoint` tag
-   */
-  @inject.binding()
-  protected binding: Binding<InvokeMiddleware>;
+export class InvokeMiddlewareProvider {
+  static value(
+    /**
+     * Inject the binding so that we can access `extensionPoint` tag
+     */
+    @inject.binding()
+    binding: Binding<InvokeMiddleware>,
 
-  /**
-   * Default options for invoking the middleware chain
-   */
-  @config()
-  protected defaultOptions: InvokeMiddlewareOptions = {
-    chain: DEFAULT_MIDDLEWARE_CHAIN,
-    orderedGroups: ['cors', 'apiSpec', DEFAULT_MIDDLEWARE_GROUP],
-  };
-
-  value(): InvokeMiddleware {
-    debug('Binding', this.binding);
-    return (
+    /**
+     * Default options for invoking the middleware chain
+     */
+    @config()
+    defaultOptions: InvokeMiddlewareOptions = {
+      chain: DEFAULT_MIDDLEWARE_CHAIN,
+      orderedGroups: ['cors', 'apiSpec', DEFAULT_MIDDLEWARE_GROUP],
+    },
+  ): InvokeMiddleware {
+    debug('Binding', binding);
+    debug('Default options', defaultOptions);
+    const invokeMiddlewareFn: InvokeMiddleware = (
       middlewareCtx: MiddlewareContext,
       optionsOrHandlers?: InvokeMiddlewareOptions | ExpressRequestHandler[],
     ) => {
@@ -57,17 +58,20 @@ export class InvokeMiddlewareProvider implements Provider<InvokeMiddleware> {
       const orderedGroups = options?.orderedGroups;
       chain =
         chain ??
-        this.binding?.tagMap[CoreTags.EXTENSION_POINT] ??
-        this.defaultOptions.chain;
-      return this.action(middlewareCtx, {
+        binding?.tagMap[CoreTags.EXTENSION_POINT] ??
+        defaultOptions.chain;
+      const middlewareOptions = {
         ...options,
         chain,
-        orderedGroups: orderedGroups ?? this.defaultOptions.orderedGroups,
-      });
+        orderedGroups: orderedGroups ?? defaultOptions.orderedGroups,
+      };
+      debug('Invoke middleware with', middlewareOptions);
+      return this.action(middlewareCtx, middlewareOptions);
     };
+    return invokeMiddlewareFn;
   }
 
-  async action(
+  static async action(
     middlewareCtx: MiddlewareContext,
     optionsOrHandlers?: InvokeMiddlewareOptions | ExpressRequestHandler[],
   ) {

diff --git a/packages/rest/src/__tests__/acceptance/middleware/middleware-sequence.acceptance.ts b/packages/rest/src/__tests__/acceptance/middleware/middleware-sequence.acceptance.ts
@@ -114,7 +114,7 @@ describe('Middleware in sequence', () => {
     // Create another middleware phase
     helper.app
       .bind('middleware.postInvoke')
-      .toProvider(InvokeMiddlewareProvider)
+      .toDynamicValue(InvokeMiddlewareProvider)
       // Configure a different extension point name
       .tag({[CoreTags.EXTENSION_POINT]: POST_INVOCATION_MIDDLEWARE});
     helper.app.sequence(SequenceWithOneInvokeMiddleware);

diff --git a/packages/rest/src/__tests__/integration/http-handler.integration.ts b/packages/rest/src/__tests__/integration/http-handler.integration.ts
@@ -619,18 +619,20 @@ describe('HttpHandler', () => {
   let handler: HttpHandler;
   function givenHandler() {
     rootContext = new Context();
-    rootContext.bind(SequenceActions.FIND_ROUTE).toProvider(FindRouteProvider);
+    rootContext
+      .bind(SequenceActions.FIND_ROUTE)
+      .toDynamicValue(FindRouteProvider);
     rootContext
       .bind(SequenceActions.PARSE_PARAMS)
-      .toProvider(ParseParamsProvider);
+      .toDynamicValue(ParseParamsProvider);
     rootContext
       .bind(SequenceActions.INVOKE_METHOD)
-      .toProvider(InvokeMethodProvider);
+      .toDynamicValue(InvokeMethodProvider);
     rootContext
       .bind(SequenceActions.LOG_ERROR)
       .to(createUnexpectedHttpErrorLogger());
     rootContext.bind(SequenceActions.SEND).to(writeResultToResponse);
-    rootContext.bind(SequenceActions.REJECT).toProvider(RejectProvider);
+    rootContext.bind(SequenceActions.REJECT).toDynamicValue(RejectProvider);
 
     rootContext.bind(RestBindings.SEQUENCE).toClass(DefaultSequence);
 

diff --git a/packages/rest/src/__tests__/unit/rest.component.unit.ts b/packages/rest/src/__tests__/unit/rest.component.unit.ts
@@ -3,12 +3,15 @@
 // This file is licensed under the MIT License.
 // License text available at https://opensource.org/licenses/MIT
 
-import {BoundValue, Context, inject, Provider} from '@loopback/core';
 import {
   Application,
+  BindingType,
   Component,
+  Context,
   CoreBindings,
-  ProviderMap,
+  createBindingFromClass,
+  inject,
+  Provider,
 } from '@loopback/core';
 import {expect, stubExpressContext} from '@loopback/testlab';
 import {
@@ -53,9 +56,9 @@ describe('RestComponent', () => {
 
       for (const key of EXPECTED_KEYS) {
         it(`binds ${key}`, async () => {
-          const result = await app.get(key);
-          const expected: Provider<BoundValue> = new comp.providers![key]();
-          expect(result).to.deepEqual(expected.value());
+          await app.get(key);
+          const expected = comp.bindings?.find(b => b.key === key);
+          expect(expected?.type).to.eql(BindingType.DYNAMIC_VALUE);
         });
       }
     });
@@ -73,9 +76,11 @@ describe('RestComponent', () => {
         let lastLog = 'logError() was not called';
 
         class CustomRestComponent extends RestComponent {
-          providers: ProviderMap = {
-            [RestBindings.SequenceActions.LOG_ERROR.key]: CustomLogger,
-          };
+          bindings = [
+            createBindingFromClass(CustomLogger, {
+              key: RestBindings.SequenceActions.LOG_ERROR,
+            }),
+          ];
           constructor(
             @inject(CoreBindings.APPLICATION_INSTANCE) application: Application,
             @inject(CoreBindings.APPLICATION_CONFIG)

diff --git a/packages/rest/src/__tests__/unit/router/reject.provider.unit.ts b/packages/rest/src/__tests__/unit/router/reject.provider.unit.ts
@@ -4,10 +4,10 @@
 // License text available at https://opensource.org/licenses/MIT
 
 import {
-  ExpressContextStub,
-  SinonSpy,
   expect,
+  ExpressContextStub,
   sinon,
+  SinonSpy,
   stubExpressContext,
 } from '@loopback/testlab';
 import {LogError, RejectProvider} from '../../..';
@@ -20,7 +20,7 @@ describe('reject', () => {
   beforeEach(givenStubbedContext);
 
   it('returns HTTP response with status code 500 by default', async () => {
-    const reject = new RejectProvider(noopLogger).value();
+    const reject = RejectProvider.value(noopLogger);
 
     reject(contextStub, testError);
     const result = await contextStub.result;
@@ -29,7 +29,7 @@ describe('reject', () => {
   });
 
   it('converts error code ENTITY_NOT_FOUND to status code 404', async () => {
-    const reject = new RejectProvider(noopLogger).value();
+    const reject = RejectProvider.value(noopLogger);
 
     const notFoundError: Error & {code?: string} = new Error('not found');
     notFoundError.code = 'ENTITY_NOT_FOUND';
@@ -42,7 +42,7 @@ describe('reject', () => {
 
   it('logs the error', async () => {
     const logger = sinon.spy() as LogError & SinonSpy;
-    const reject = new RejectProvider(logger).value();
+    const reject = RejectProvider.value(logger);
 
     reject(contextStub, testError);
     await contextStub.result;

diff --git a/packages/rest/src/providers/find-route.provider.ts b/packages/rest/src/providers/find-route.provider.ts
@@ -8,27 +8,28 @@ import {asMiddleware, Middleware} from '@loopback/express';
 import debugFactory from 'debug';
 import {HttpHandler} from '../http-handler';
 import {RestBindings, RestTags} from '../keys';
-import {ResolvedRoute} from '../router';
 import {RestMiddlewareGroups} from '../sequence';
-import {FindRoute, Request} from '../types';
+import {FindRoute} from '../types';
 
 const debug = debugFactory('loopback:rest:find-route');
 
-export class FindRouteProvider implements Provider<FindRoute> {
-  constructor(
-    @inject(RestBindings.Http.CONTEXT) protected context: Context,
-    @inject(RestBindings.HANDLER) protected handler: HttpHandler,
-  ) {}
-
-  value(): FindRoute {
-    return request => this.action(request);
-  }
-
-  action(request: Request): ResolvedRoute {
-    const found = this.handler.findRoute(request);
-    debug('Route found for %s %s', request.method, request.originalUrl, found);
-    found.updateBindings(this.context);
-    return found;
+export class FindRouteProvider {
+  static value(
+    @inject(RestBindings.Http.CONTEXT) context: Context,
+    @inject(RestBindings.HANDLER) handler: HttpHandler,
+  ): FindRoute {
+    const findRoute: FindRoute = request => {
+      const found = handler.findRoute(request);
+      debug(
+        'Route found for %s %s',
+        request.method,
+        request.originalUrl,
+        found,
+      );
+      found.updateBindings(context);
+      return found;
+    };
+    return findRoute;
   }
 }
 

diff --git a/packages/rest/src/providers/invoke-method.provider.ts b/packages/rest/src/providers/invoke-method.provider.ts
@@ -9,19 +9,17 @@ import debugFactory from 'debug';
 import {RestBindings, RestTags} from '../keys';
 import {RouteEntry} from '../router';
 import {RestMiddlewareGroups} from '../sequence';
-import {InvokeMethod, OperationArgs, OperationRetval} from '../types';
+import {InvokeMethod, OperationArgs} from '../types';
 
 const debug = debugFactory('loopback:rest:invoke-method');
 
-export class InvokeMethodProvider implements Provider<InvokeMethod> {
-  constructor(@inject(RestBindings.Http.CONTEXT) protected context: Context) {}
-
-  value(): InvokeMethod {
-    return (route, args) => this.action(route, args);
-  }
-
-  action(route: RouteEntry, args: OperationArgs): Promise<OperationRetval> {
-    return route.invokeHandler(this.context, args);
+export class InvokeMethodProvider {
+  static value(
+    @inject(RestBindings.Http.CONTEXT) context: Context,
+  ): InvokeMethod {
+    const invokeMethod: InvokeMethod = (route, args) =>
+      route.invokeHandler(context, args);
+    return invokeMethod;
   }
 }
 

diff --git a/packages/rest/src/providers/log-error.provider.ts b/packages/rest/src/providers/log-error.provider.ts
@@ -3,25 +3,23 @@
 // This file is licensed under the MIT License.
 // License text available at https://opensource.org/licenses/MIT
 
-import {Provider} from '@loopback/core';
-import {LogError, Request} from '../types';
+import {LogError} from '../types';
 
-export class LogErrorProvider implements Provider<LogError> {
-  value(): LogError {
-    return (err, statusCode, req) => this.action(err, statusCode, req);
-  }
-
-  action(err: Error, statusCode: number, req: Request) {
-    if (statusCode < 500) {
-      return;
-    }
+export class LogErrorProvider {
+  static value(): LogError {
+    const logError: LogError = (err, statusCode, req) => {
+      if (statusCode < 500) {
+        return;
+      }
 
-    console.error(
-      'Unhandled error in %s %s: %s %s',
-      req.method,
-      req.url,
-      statusCode,
-      err.stack ?? err,
-    );
+      console.error(
+        'Unhandled error in %s %s: %s %s',
+        req.method,
+        req.url,
+        statusCode,
+        err.stack ?? err,
+      );
+    };
+    return logError;
   }
 }
diff --git a/packages/rest/src/providers/parse-params.provider.ts b/packages/rest/src/providers/parse-params.provider.ts
@@ -21,25 +21,24 @@ const debug = debugFactory('loopback:rest:parse-param');
  *
  * @returns The handler function that will parse request args.
  */
-export class ParseParamsProvider implements Provider<ParseParams> {
-  constructor(
+export class ParseParamsProvider {
+  static value(
     @inject(RestBindings.REQUEST_BODY_PARSER)
-    private requestBodyParser: RequestBodyParser,
+    requestBodyParser: RequestBodyParser,
     @inject(
       RestBindings.REQUEST_BODY_PARSER_OPTIONS.deepProperty('validation'),
       {optional: true},
     )
-    private validationOptions: ValidationOptions = DEFAULT_AJV_VALIDATION_OPTIONS,
+    validationOptions: ValidationOptions = DEFAULT_AJV_VALIDATION_OPTIONS,
     @inject(RestBindings.AJV_FACTORY, {optional: true})
-    private ajvFactory?: AjvFactory,
-  ) {}
-
-  value(): ParseParams {
-    return (request: Request, route: ResolvedRoute) =>
-      parseOperationArgs(request, route, this.requestBodyParser, {
-        ajvFactory: this.ajvFactory,
-        ...this.validationOptions,
+    ajvFactory: AjvFactory,
+  ): ParseParams {
+    const parseParams: ParseParams = (request: Request, route: ResolvedRoute) =>
+      parseOperationArgs(request, route, requestBodyParser, {
+        ajvFactory: ajvFactory,
+        ...validationOptions,
       });
+    return parseParams;
   }
 }