From 01867561162d0186424c858354a98e20bf3553e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Tue, 12 May 2020 11:11:04 +0200 Subject: [PATCH] refactor: enable type checks inside mixin classes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce a new type helper `MixinTarget` allowing mixin functions to accept a type that describes public members of the target class only. This is working around the current TypeScript limitations. Rework all existing mixins and the related documentation to use `MixinTarget` instead of `Constructor`. Fix any errors discovered by the compiler after enabling type checks. Signed-off-by: Miroslav Bajtoš --- docs/site/Creating-components.md | 2 +- docs/site/Mixin.md | 4 +- docs/site/Testing-Your-Extensions.md | 2 +- docs/site/migration/models/mixins.md | 14 ++-- examples/log-extension/README.md | 5 +- .../log-extension/src/mixins/log.mixin.ts | 7 +- packages/boot/src/mixins/boot.mixin.ts | 50 ++++++++----- packages/core/src/core.types.ts | 70 +++++++++++++++++++ packages/core/src/index.ts | 1 + packages/express/src/middleware-registry.ts | 3 +- .../express/src/mixins/middleware.mixin.ts | 4 +- .../mixins/category-property-mixin.ts | 4 +- .../mixins/find-by-title-repo-mixin.ts | 4 +- .../repository/src/mixins/repository.mixin.ts | 27 ++++--- .../mixins/find-by-title-controller-mixin.ts | 8 ++- 15 files changed, 150 insertions(+), 55 deletions(-) create mode 100644 packages/core/src/core.types.ts diff --git a/docs/site/Creating-components.md b/docs/site/Creating-components.md index eda3871623b6..289c44f6fd56 100644 --- a/docs/site/Creating-components.md +++ b/docs/site/Creating-components.md @@ -364,7 +364,7 @@ The following snippet is an abbreviated function {% include code-caption.html content="src/mixins/repository.mixin.ts" %} ```ts -export function RepositoryMixin>(superClass: T) { +export function RepositoryMixin>(superClass: T) { return class extends superClass { constructor(...args: any[]) { super(...args); diff --git a/docs/site/Mixin.md b/docs/site/Mixin.md index 64cd48f6f765..3d3fdec10f23 100644 --- a/docs/site/Mixin.md +++ b/docs/site/Mixin.md @@ -54,7 +54,7 @@ Define mixin `TimeStampMixin`: ```ts import {Class} from '@loopback/repository'; -export function TimeStampMixin>(baseClass: T) { +export function TimeStampMixin>(baseClass: T) { return class extends baseClass { // add a new property `createdAt` public createdAt: Date; @@ -76,7 +76,7 @@ And define mixin `LoggerMixin`: ```ts import {Class} from '@loopback/repository'; -function LoggerMixin>(baseClass: T) { +function LoggerMixin>(baseClass: T) { return class extends baseClass { // add a new method `log()` log(str: string) { diff --git a/docs/site/Testing-Your-Extensions.md b/docs/site/Testing-Your-Extensions.md index 5582b1caa227..e2cfdd64d8ba 100644 --- a/docs/site/Testing-Your-Extensions.md +++ b/docs/site/Testing-Your-Extensions.md @@ -234,7 +234,7 @@ class. Following is an example for an integration test for a Mixin: ```ts import {Constructor} from '@loopback/context'; -export function TimeMixin>(superClass: T) { +export function TimeMixin>(superClass: T) { return class extends superClass { constructor(...args: any[]) { super(...args); diff --git a/docs/site/migration/models/mixins.md b/docs/site/migration/models/mixins.md index 86b9144e6b55..a5e50d57182f 100644 --- a/docs/site/migration/models/mixins.md +++ b/docs/site/migration/models/mixins.md @@ -16,7 +16,7 @@ This document will guide you in migrating custom model mixins, and custom method/remote method mixins in LoopBack 3 to their equivalent implementations in LoopBack 4. -For an understanding of how models in LoopBack 3 are now architectually +For an understanding of how models in LoopBack 3 are now architecturally decoupled into 3 classes (model, repository, and controller) please read [Migrating custom model methods](./methods.md). @@ -169,7 +169,7 @@ import {property, Model} from '@loopback/repository'; * @param superClass - Base Class * @typeParam T - Model class */ -export function AddCategoryPropertyMixin>( +export function AddCategoryPropertyMixin>( superClass: T, ) { class MixedModel extends superClass { @@ -447,7 +447,7 @@ import {FindByTitle} from './find-by-title-interface'; */ export function FindByTitleRepositoryMixin< M extends Model & {title: string}, - R extends Constructor> + R extends MixinTarget> >(superClass: R) { class MixedRepository extends superClass implements FindByTitle { async findByTitle(title: string): Promise { @@ -552,10 +552,12 @@ export interface FindByTitleControllerMixinOptions { */ export function FindByTitleControllerMixin< M extends Model, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - T extends Constructor = Constructor + T extends MixinTarget >(superClass: T, options: FindByTitleControllerMixinOptions) { class MixedController extends superClass implements FindByTitle { + // Value will be provided by the subclassed controller class + repository: FindByTitle; + @get(`${options.basePath}/findByTitle/{title}`, { responses: { '200': { @@ -587,7 +589,7 @@ mixin class factory function needs to accept some options. We defined an interface `FindByTitleControllerMixinOptions` to allow for this. It is also a good idea to give the injected repository (in the controller super -class) a generic name like `this.respository` to keep things simple in the mixin +class) a generic name like `this.repository` to keep things simple in the mixin class factory function. #### Generating A Controller Via The CLI diff --git a/examples/log-extension/README.md b/examples/log-extension/README.md index 1e9b231e6124..373c7c3cbf84 100644 --- a/examples/log-extension/README.md +++ b/examples/log-extension/README.md @@ -228,12 +228,11 @@ providing it via `ApplicationOptions` or using a helper method `app.logLevel(level: number)`. ```ts -import {Constructor} from '@loopback/context'; +import {MixinTarget, Application} from '@loopback/core'; import {EXAMPLE_LOG_BINDINGS} from '../keys'; import {LogComponent} from '../component'; -// eslint-disable-next-line @typescript-eslint/no-explicit-any -export function LogMixin>(superClass: T) { +export function LogMixin>(superClass: T) { return class extends superClass { // eslint-disable-next-line @typescript-eslint/no-explicit-any constructor(...args: any[]) { diff --git a/examples/log-extension/src/mixins/log.mixin.ts b/examples/log-extension/src/mixins/log.mixin.ts index d5f00b8d38b3..c54d1429783f 100644 --- a/examples/log-extension/src/mixins/log.mixin.ts +++ b/examples/log-extension/src/mixins/log.mixin.ts @@ -3,9 +3,9 @@ // This file is licensed under the MIT License. // License text available at https://opensource.org/licenses/MIT -import {Constructor} from '@loopback/context'; -import {EXAMPLE_LOG_BINDINGS, LOG_LEVEL} from '../keys'; +import {Application, MixinTarget} from '@loopback/core'; import {LogComponent} from '../component'; +import {EXAMPLE_LOG_BINDINGS, LOG_LEVEL} from '../keys'; /** * A mixin class for Application that can bind logLevel from `options`. @@ -17,8 +17,7 @@ import {LogComponent} from '../component'; * class MyApplication extends LogMixin(Application) {} * ``` */ -// eslint-disable-next-line @typescript-eslint/no-explicit-any -export function LogMixin>(superClass: T) { +export function LogMixin>(superClass: T) { return class extends superClass { // A mixin class has to take in a type any[] argument! // eslint-disable-next-line @typescript-eslint/no-explicit-any diff --git a/packages/boot/src/mixins/boot.mixin.ts b/packages/boot/src/mixins/boot.mixin.ts index 40d36333fea9..9a58195cc9c3 100644 --- a/packages/boot/src/mixins/boot.mixin.ts +++ b/packages/boot/src/mixins/boot.mixin.ts @@ -10,6 +10,7 @@ import { Context, createBindingFromClass, } from '@loopback/context'; +import {Application, MixinTarget} from '@loopback/core'; import {BootComponent} from '../boot.component'; import {Bootstrapper} from '../bootstrapper'; import {BootBindings, BootTags} from '../keys'; @@ -30,18 +31,8 @@ export {Binding}; * - Provides the `booter()` convenience method to bind a Booter(s) to the Application * - Override `component()` to call `mountComponentBooters` * - Adds `mountComponentBooters` which binds Booters to the application from `component.booters[]` - * - * ******************** NOTE ******************** - * Trying to constrain the type of this Mixin (or any Mixin) will cause errors. - * For example, constraining this Mixin to type Application require all types using by - * Application to be imported (including it's dependencies such as ResolutionSession). - * Another issue was that if a Mixin that is type constrained is used with another Mixin - * that is not, it will result in an error. - * Example (class MyApp extends BootMixin(RepositoryMixin(Application))) {}; - ********************* END OF NOTE ******************** */ -// eslint-disable-next-line @typescript-eslint/no-explicit-any -export function BootMixin>(superClass: T) { +export function BootMixin>(superClass: T) { return class extends superClass implements Bootable { projectRoot: string; bootOptions?: BootOptions; @@ -57,7 +48,7 @@ export function BootMixin>(superClass: T) { () => this.projectRoot, ); this.bind(BootBindings.BOOT_OPTIONS).toDynamicValue( - () => this.bootOptions, + () => this.bootOptions ?? {}, ); } @@ -65,18 +56,34 @@ export function BootMixin>(superClass: T) { * Convenience method to call bootstrapper.boot() by resolving bootstrapper */ async boot(): Promise { - if (this.state === 'booting') return this.awaitState('booted'); - this.assertNotInProcess('boot'); - this.assertInStates('boot', 'created', 'booted'); + /* eslint-disable @typescript-eslint/ban-ts-ignore */ + // A workaround to access protected Application methods + const self = (this as unknown) as Application; + + if (this.state === 'booting') { + // @ts-ignore + return self.awaitState('booted'); + } + // @ts-ignore + self.assertNotInProcess('boot'); + // @ts-ignore + self.assertInStates('boot', 'created', 'booted'); + if (this.state === 'booted') return; - this.setState('booting'); + // @ts-ignore + self.setState('booting'); + // Get a instance of the BootStrapper const bootstrapper: Bootstrapper = await this.get( BootBindings.BOOTSTRAPPER_KEY, ); await bootstrapper.boot(); + + // @ts-ignore this.setState('booted'); + + /* eslint-enable @typescript-eslint/ban-ts-ignore */ } /** @@ -115,9 +122,14 @@ export function BootMixin>(superClass: T) { * app.component(ProductComponent); * ``` */ + // Unfortunately, TypeScript does not allow overriding methods inherited + // from mapped types. https://github.com/microsoft/TypeScript/issues/38496 + // eslint-disable-next-line @typescript-eslint/ban-ts-ignore + // @ts-ignore public component(component: Constructor<{}>) { - super.component(component); + const binding = super.component(component); this.mountComponentBooters(component); + return binding; } /** @@ -129,7 +141,9 @@ export function BootMixin>(superClass: T) { */ mountComponentBooters(component: Constructor<{}>) { const componentKey = `components.${component.name}`; - const compInstance = this.getSync(componentKey); + const compInstance = this.getSync<{ + booters?: Constructor[]; + }>(componentKey); if (compInstance.booters) { this.booters(...compInstance.booters); diff --git a/packages/core/src/core.types.ts b/packages/core/src/core.types.ts new file mode 100644 index 000000000000..2432249d597b --- /dev/null +++ b/packages/core/src/core.types.ts @@ -0,0 +1,70 @@ +// Copyright IBM Corp. 2017,2020. All Rights Reserved. +// Node module: @loopback/core +// This file is licensed under the MIT License. +// License text available at https://opensource.org/licenses/MIT + +import {Constructor} from '@loopback/context'; + +/** + * A replacement for `typeof Target` to be used in mixin class definitions. + * This is a workaround for TypeScript limitation described in + * - https://github.com/microsoft/TypeScript/issues/17293 + * - https://github.com/microsoft/TypeScript/issues/17744 + * - https://github.com/microsoft/TypeScript/issues/36060 + * + * @example + * + * ```ts + * export function MyMixin>(superClass: T) { + * return class extends superClass { + * // contribute new class members + * } + * }; + * ``` + * + * TypeScript does not allow class mixins to access protected members from + * the base class. You can use the following approach as a workaround: + * + * ```ts + * // @ts-ignore + * (this as unknown as {YourBaseClass}).protectedMember + * ``` + * + * The directive `@ts-ignore` suppresses compiler error about accessing + * a protected member from outside. Unfortunately, it also disables other + * compile-time checks (e.g. to verify that a protected method was invoked + * with correct arguments, and so on). This is the same behavior you + * would get by using `Constructor` instead of `MixinTarget`. + * The major improvement is that TypeScript can still infer the return + * type of the protected member, therefore `any` is NOT introduced to subsequent + * code. + * + * TypeScript also does not allow mixin class to overwrite a method inherited + * from a mapped type, see https://github.com/microsoft/TypeScript/issues/38496 + * As a workaround, use `@ts-ignore` to disable the error. + * + * ```ts + * export function RepositoryMixin>( + * superClass: T, + * ) { + * return class extends superClass { + * // @ts-ignore + * public component( + * componentCtor: Constructor, + * nameOrOptions?: string | BindingFromClassOptions, + * ) { + * const binding = super.component(componentCtor, nameOrOptions); + * // ... + * return binding; + * } + * } + * ``` + */ +export type MixinTarget = Constructor< + { + // Enumerate only public members to avoid the following compiler error: + // Property '(name)' of exported class expression + // may not be private or protected.ts(4094) + [p in keyof T]: T[p]; + } +>; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index a3742b763692..4dd71171a074 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -20,6 +20,7 @@ export * from '@loopback/context'; // Export APIs export * from './application'; export * from './component'; +export * from './core.types'; export * from './extension-point'; export * from './keys'; export * from './lifecycle'; diff --git a/packages/express/src/middleware-registry.ts b/packages/express/src/middleware-registry.ts index db10fb5607bb..53ae2b24ec04 100644 --- a/packages/express/src/middleware-registry.ts +++ b/packages/express/src/middleware-registry.ts @@ -88,5 +88,4 @@ export interface MiddlewareRegistry { /** * Base Context that provides APIs to register middleware */ -export abstract class BaseMiddlewareRegistry extends MiddlewareMixin(Context) - implements MiddlewareRegistry {} +export abstract class BaseMiddlewareRegistry extends MiddlewareMixin(Context) {} diff --git a/packages/express/src/mixins/middleware.mixin.ts b/packages/express/src/mixins/middleware.mixin.ts index 85544d16a18c..4baf662a0379 100644 --- a/packages/express/src/mixins/middleware.mixin.ts +++ b/packages/express/src/mixins/middleware.mixin.ts @@ -9,6 +9,7 @@ import { Constructor, Context, isBindingAddress, + MixinTarget, Provider, } from '@loopback/core'; import { @@ -36,8 +37,7 @@ function extendsFrom( return false; } -// eslint-disable-next-line @typescript-eslint/no-explicit-any -export function MiddlewareMixin>(superClass: T) { +export function MiddlewareMixin>(superClass: T) { if (!extendsFrom(superClass, Context)) { throw new TypeError('The super class does not extend from Context'); } diff --git a/packages/repository/src/__tests__/fixtures/mixins/category-property-mixin.ts b/packages/repository/src/__tests__/fixtures/mixins/category-property-mixin.ts index 686c4042373b..1d133d319548 100644 --- a/packages/repository/src/__tests__/fixtures/mixins/category-property-mixin.ts +++ b/packages/repository/src/__tests__/fixtures/mixins/category-property-mixin.ts @@ -3,7 +3,7 @@ // This file is licensed under the MIT License. // License text available at https://opensource.org/licenses/MIT -import {Constructor} from '@loopback/context'; +import {MixinTarget} from '@loopback/core'; import {Model, property} from '../../..'; /** @@ -12,7 +12,7 @@ import {Model, property} from '../../..'; * @param superClass - Base Class * @typeParam T - Model class */ -export function AddCategoryPropertyMixin>( +export function AddCategoryPropertyMixin>( superClass: T, ) { class MixedModel extends superClass { diff --git a/packages/repository/src/__tests__/fixtures/mixins/find-by-title-repo-mixin.ts b/packages/repository/src/__tests__/fixtures/mixins/find-by-title-repo-mixin.ts index 92ecc2765c88..f788a2bef6fb 100644 --- a/packages/repository/src/__tests__/fixtures/mixins/find-by-title-repo-mixin.ts +++ b/packages/repository/src/__tests__/fixtures/mixins/find-by-title-repo-mixin.ts @@ -3,7 +3,7 @@ // This file is licensed under the MIT License. // License text available at https://opensource.org/licenses/MIT -import {Constructor} from '@loopback/context'; +import {MixinTarget} from '@loopback/core'; import {CrudRepository, Model, Where} from '../../..'; /** @@ -24,7 +24,7 @@ export interface FindByTitle { */ export function FindByTitleRepositoryMixin< M extends Model & {title: string}, - R extends Constructor> + R extends MixinTarget> >(superClass: R) { class MixedRepository extends superClass implements FindByTitle { async findByTitle(title: string): Promise { diff --git a/packages/repository/src/mixins/repository.mixin.ts b/packages/repository/src/mixins/repository.mixin.ts index 38b8ef65e6b5..db149636ff8b 100644 --- a/packages/repository/src/mixins/repository.mixin.ts +++ b/packages/repository/src/mixins/repository.mixin.ts @@ -9,10 +9,11 @@ import { BindingScope, createBindingFromClass, } from '@loopback/context'; -import {Application} from '@loopback/core'; +import {Application, Component, Constructor, MixinTarget} from '@loopback/core'; import debugFactory from 'debug'; import {Class} from '../common-types'; import {SchemaMigrationOptions} from '../datasource'; +import {Model} from '../model'; import {juggler, Repository} from '../repositories'; const debug = debugFactory('loopback:repository:mixin'); @@ -31,8 +32,9 @@ const debug = debugFactory('loopback:repository:mixin'); * called RepositoryMixinDoc * */ -// eslint-disable-next-line @typescript-eslint/no-explicit-any -export function RepositoryMixin>(superClass: T) { +export function RepositoryMixin>( + superClass: T, +) { return class extends superClass { /** * Add a repository to this application. @@ -161,12 +163,17 @@ export function RepositoryMixin>(superClass: T) { * app.component(ProductComponent); * ``` */ - public component( - component: Class, + // Unfortunately, TypeScript does not allow overriding methods inherited + // from mapped types. https://github.com/microsoft/TypeScript/issues/38496 + // eslint-disable-next-line @typescript-eslint/ban-ts-ignore + // @ts-ignore + public component( + componentCtor: Constructor, nameOrOptions?: string | BindingFromClassOptions, ) { - super.component(component, nameOrOptions); - this.mountComponentRepositories(component); + const binding = super.component(componentCtor, nameOrOptions); + this.mountComponentRepositories(componentCtor); + return binding; } /** @@ -178,7 +185,9 @@ export function RepositoryMixin>(superClass: T) { */ mountComponentRepositories(component: Class) { const componentKey = `components.${component.name}`; - const compInstance = this.getSync(componentKey); + const compInstance = this.getSync<{ + repositories?: Class>[]; + }>(componentKey); if (compInstance.repositories) { for (const repo of compInstance.repositories) { @@ -216,7 +225,7 @@ export function RepositoryMixin>(superClass: T) { 'datasource', ); for (const b of dsBindings) { - const ds = await this.get(b.key); + const ds = await this.get(b.key); if (operation in ds && typeof ds[operation] === 'function') { debug('Migrating dataSource %s', b.key); diff --git a/packages/rest/src/__tests__/fixtures/mixins/find-by-title-controller-mixin.ts b/packages/rest/src/__tests__/fixtures/mixins/find-by-title-controller-mixin.ts index 24364a928acf..d285047f785a 100644 --- a/packages/rest/src/__tests__/fixtures/mixins/find-by-title-controller-mixin.ts +++ b/packages/rest/src/__tests__/fixtures/mixins/find-by-title-controller-mixin.ts @@ -3,7 +3,7 @@ // This file is licensed under the MIT License. // License text available at https://opensource.org/licenses/MIT -import {Constructor} from '@loopback/context'; +import {MixinTarget} from '@loopback/core'; import {Model} from '@loopback/repository'; import {get, getModelSchemaRef, param} from '../../../'; @@ -38,10 +38,12 @@ export interface FindByTitleControllerMixinOptions { */ export function FindByTitleControllerMixin< M extends Model, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - T extends Constructor = Constructor + T extends MixinTarget >(superClass: T, options: FindByTitleControllerMixinOptions) { class MixedController extends superClass implements FindByTitle { + // Value will be provided by the subclassed controller class + repository: FindByTitle; + @get(`${options.basePath}/findByTitle/{title}`, { responses: { '200': {