From 55cd0f249509cc6ee94e6781ffd902b0e464b75d Mon Sep 17 00:00:00 2001 From: Noel De Martin Date: Mon, 15 Mar 2021 14:31:00 +0100 Subject: [PATCH] MOBILE-3720 DX: Use Proxy for magic singletons --- src/addons/calendar/services/calendar-sync.ts | 2 +- src/addons/messages/services/messages-sync.ts | 2 +- src/addons/mod/assign/services/assign-sync.ts | 2 +- src/addons/mod/forum/services/sync.ts | 2 +- src/addons/mod/lesson/services/lesson-sync.ts | 2 +- src/addons/mod/quiz/services/quiz-sync.ts | 2 +- src/core/directives/tests/format-text.test.ts | 4 +- .../features/block/services/block-delegate.ts | 2 +- .../comments/services/comments-sync.ts | 2 +- .../course/services/handlers/log-cron.ts | 2 +- src/core/features/course/services/sync.ts | 2 +- src/core/features/h5p/services/h5p.ts | 9 +- .../siteplugins/services/siteplugins.ts | 2 +- src/core/features/user/services/user-sync.ts | 2 +- src/core/services/app.ts | 2 +- src/core/services/screen.ts | 9 +- src/core/services/tests/utils/text.test.ts | 4 +- src/core/singletons/index.ts | 152 ++++++------------ src/core/singletons/tests/singletons.test.ts | 16 +- src/core/singletons/tests/stubs.ts | 20 +++ src/testing/setup.ts | 11 ++ src/testing/utils.ts | 21 ++- 22 files changed, 121 insertions(+), 151 deletions(-) diff --git a/src/addons/calendar/services/calendar-sync.ts b/src/addons/calendar/services/calendar-sync.ts index c75e4834a..e0e44e6f6 100644 --- a/src/addons/calendar/services/calendar-sync.ts +++ b/src/addons/calendar/services/calendar-sync.ts @@ -297,7 +297,7 @@ export class AddonCalendarSyncProvider extends CoreSyncBaseProvider { // @todo this is done because we cannot mock image being loaded, we should find an alternative... CoreUtils.instance.timeoutPromise = () => Promise.resolve(null as unknown as T); - mockSingleton(CoreFilepool, { getSrcByUrl: jest.fn(() => Promise.resolve('file://local-path')) }); + mockSingleton(CoreFilepool, { getSrcByUrl: () => Promise.resolve('file://local-path') }); mockSingleton(CoreSites, { - getSite: jest.fn(() => Promise.resolve(site)), + getSite: () => Promise.resolve(site), getCurrentSite: () => Promise.resolve(site), }); mockSingleton(CoreFilter, { formatText: (text) => Promise.resolve(text) }); diff --git a/src/core/features/block/services/block-delegate.ts b/src/core/features/block/services/block-delegate.ts index 403584f84..eeb0e8832 100644 --- a/src/core/features/block/services/block-delegate.ts +++ b/src/core/features/block/services/block-delegate.ts @@ -198,4 +198,4 @@ export class CoreBlockDelegateService extends CoreDelegate { } -export const CoreBlockDelegate = makeSingleton(CoreBlockDelegateService, ['blocksUpdateObservable']); +export const CoreBlockDelegate = makeSingleton(CoreBlockDelegateService); diff --git a/src/core/features/comments/services/comments-sync.ts b/src/core/features/comments/services/comments-sync.ts index be1974e52..1c91862d6 100644 --- a/src/core/features/comments/services/comments-sync.ts +++ b/src/core/features/comments/services/comments-sync.ts @@ -315,7 +315,7 @@ export class CoreCommentsSyncProvider extends CoreSyncBaseProvider { } -export const CoreUserSync = makeSingleton(CoreUserSyncProvider, ['component', 'syncInterval']); +export const CoreUserSync = makeSingleton(CoreUserSyncProvider); diff --git a/src/core/services/app.ts b/src/core/services/app.ts index abc92e0b4..f0e715315 100644 --- a/src/core/services/app.ts +++ b/src/core/services/app.ts @@ -644,7 +644,7 @@ export class CoreAppProvider { } -export const CoreApp = makeSingleton(CoreAppProvider, ['isAndroid']); +export const CoreApp = makeSingleton(CoreAppProvider); /** * Data stored for a redirect to another page/site. diff --git a/src/core/services/screen.ts b/src/core/services/screen.ts index fbb69ea6c..696abb6cb 100644 --- a/src/core/services/screen.ts +++ b/src/core/services/screen.ts @@ -140,11 +140,4 @@ export class CoreScreenService { } -export const CoreScreen = makeSingleton(CoreScreenService, [ - 'isTablet', - 'isMobile', - 'layout', - 'layoutObservable', - 'breakpoints', - 'breakpointsObservable', -]); +export const CoreScreen = makeSingleton(CoreScreenService); diff --git a/src/core/services/tests/utils/text.test.ts b/src/core/services/tests/utils/text.test.ts index 6fbc20848..1c968dbce 100644 --- a/src/core/services/tests/utils/text.test.ts +++ b/src/core/services/tests/utils/text.test.ts @@ -26,9 +26,9 @@ describe('CoreTextUtilsProvider', () => { let textUtils: CoreTextUtilsProvider; beforeEach(() => { - mockSingleton(CoreApp, [], { isAndroid: jest.fn(() => config.platform === 'android') }); + mockSingleton(CoreApp, [], { isAndroid: () => config.platform === 'android' }); - sanitizer = mock([], { bypassSecurityTrustUrl: jest.fn(url => url) }); + sanitizer = mock([], { bypassSecurityTrustUrl: url => url }); textUtils = new CoreTextUtilsProvider(sanitizer); }); diff --git a/src/core/singletons/index.ts b/src/core/singletons/index.ts index d6628b992..6d15cef42 100644 --- a/src/core/singletons/index.ts +++ b/src/core/singletons/index.ts @@ -56,39 +56,26 @@ import { Zip as ZipService } from '@ionic-native/zip/ngx'; import { TranslateService } from '@ngx-translate/core'; -const OBJECT_PROTOTYPE = Object.getPrototypeOf(Object); - /** * Injector instance used to resolve singletons. */ let singletonsInjector: Injector | null = null; /** - * Helper to get service class properties that are methods. + * Helper to create a method that proxies calls to the underlying singleton instance. */ -type GetMethods = { - [K in keyof T]: T[K] extends (...args: unknown[]) => unknown ? K : never -}[keyof T]; - -/** - * Helper to get service class properties that are not methods. - */ -type GetNonMethods = { - [K in keyof T]: T[K] extends (...args: unknown[]) => unknown ? never : K -}[keyof T]; +// eslint-disable-next-line +let createSingletonMethodProxy = (instance: any, method: Function, property: string | number | symbol) => method.bind(instance); /** * Singleton proxy created using the factory method. * * @see makeSingleton */ -export type CoreSingletonProxy = - Pick, GetNonMethods>> & - Pick & - { - instance: Service; - setInstance(instance: Service): void; - }; +export type CoreSingletonProxy = Service & { + instance: Service; + setInstance(instance: Service): void; +}; /** * Set the injector that will be used to resolve instances in the singletons of this module. @@ -99,6 +86,15 @@ export function setSingletonsInjector(injector: Injector): void { singletonsInjector = injector; } +/** + * Set the method to create method proxies. + * + * @param method Method. + */ +export function setCreateSingletonMethodProxy(method: typeof createSingletonMethodProxy): void { + createSingletonMethodProxy = method; +} + /** * Make a singleton proxy for the given injection token. * @@ -112,26 +108,19 @@ export function setSingletonsInjector(injector: Injector): void { * @param getters Getter names to proxy. * @return Singleton proxy. */ -export function makeSingleton(injectionToken: Type | Type | string): CoreSingletonProxy; -export function makeSingleton( +export function makeSingleton( // eslint-disable-line @typescript-eslint/ban-types injectionToken: Type | Type | string, - getters: Getters[], -): CoreSingletonProxy; -export function makeSingleton( - injectionToken: Type | Type | string, - getters: Getters[] = [], -): CoreSingletonProxy { - // Define instance manipulation affordances. - const proxy = { +): CoreSingletonProxy { + const singleton = { setInstance(instance: Service) { - Object.defineProperty(proxy, 'instance', { + Object.defineProperty(singleton, 'instance', { value: instance, configurable: true, }); }, - } as CoreSingletonProxy; + } as { instance: Service; setInstance(instance: Service) }; - Object.defineProperty(proxy, 'instance', { + Object.defineProperty(singleton, 'instance', { get: () => { if (!singletonsInjector) { throw new Error('Can\'t resolve a singleton instance without an injector'); @@ -139,70 +128,39 @@ export function makeSingleton( const instance = singletonsInjector.get(injectionToken); - proxy.setInstance(instance); + singleton.setInstance(instance); return instance; }, configurable: true, }); - // Define method and getter proxies. - if (isServiceClass(injectionToken)) { - // Get property descriptors, going all the way up the prototype chain (for services extending other classes). - let parentPrototype = injectionToken; - let descriptors: Record = {}; - - do { - descriptors = { - ...Object.getOwnPropertyDescriptors(parentPrototype.prototype), - ...descriptors, - }; - - parentPrototype = Object.getPrototypeOf(parentPrototype); - } while (parentPrototype !== OBJECT_PROTOTYPE); - - // Don't proxy constructor calls. - delete descriptors['constructor']; - - // Define method proxies. - for (const [property, descriptor] of Object.entries(descriptors)) { - // Skip getters and setters. - if (descriptor.get || descriptor.set) { - continue; + return new Proxy(singleton, { + get(target, property, receiver) { + if (property in target) { + return Reflect.get(target, property, receiver); } - // Define method proxy. - Object.defineProperty(proxy, property, { - value: (...args) => proxy.instance[property].call(proxy.instance, ...args), - configurable: true, - }); - } + const value = target.instance[property]; - // Define getter proxies. - for (const getter of getters) { - Object.defineProperty(proxy, getter, { get: () => proxy.instance[getter] }); - } - } + return typeof value === 'function' + ? createSingletonMethodProxy(target.instance, value, property) + : value; + }, + set(target, property, value, receiver) { + Reflect.set(target.instance, property, value, receiver); - return proxy; -} - -/** - * Type guard to check if an injection token is a service class. - * - * @param injectionToken Injection token. - * @return Whether the token is a class. - */ -function isServiceClass(injectionToken: Type | string): injectionToken is Type { - return typeof injectionToken !== 'string'; + return true; + }, + }) as CoreSingletonProxy; } // Convert ionic-native services to singleton. export const Badge = makeSingleton(BadgeService); export const Chooser = makeSingleton(ChooserService); export const Clipboard = makeSingleton(ClipboardService); -export const Diagnostic = makeSingleton(DiagnosticService, ['permissionStatus']); -export const File = makeSingleton(FileService, ['documentsDirectory', 'externalApplicationStorageDirectory']); +export const Diagnostic = makeSingleton(DiagnosticService); +export const File = makeSingleton(FileService); export const FileOpener = makeSingleton(FileOpenerService); export const FileTransfer = makeSingleton(FileTransferService); export const Geolocation = makeSingleton(GeolocationService); @@ -212,40 +170,24 @@ export const LocalNotifications = makeSingleton(LocalNotificationsService); export const Media = makeSingleton(MediaService); export const MediaCapture = makeSingleton(MediaCaptureService); export const NativeHttp = makeSingleton(HTTP); -export const Network = makeSingleton(NetworkService, ['Connection', 'type']); +export const Network = makeSingleton(NetworkService); export const Push = makeSingleton(PushService); export const QRScanner = makeSingleton(QRScannerService); export const StatusBar = makeSingleton(StatusBarService); export const SplashScreen = makeSingleton(SplashScreenService); export const SQLite = makeSingleton(SQLiteService); -export const WebIntent = makeSingleton(WebIntentService, ['ACTION_VIEW']); +export const WebIntent = makeSingleton(WebIntentService); export const WebView = makeSingleton(WebViewService); export const Zip = makeSingleton(ZipService); -export const Camera = makeSingleton(CameraService, [ - 'DestinationType', - 'Direction', - 'EncodingType', - 'MediaType', - 'PictureSourceType', - 'PopoverArrowDirection', -]); +export const Camera = makeSingleton(CameraService); -export const Device = makeSingleton(DeviceService, [ - 'cordova', - 'isVirtual', - 'manufacturer', - 'model', - 'platform', - 'serial', - 'uuid', - 'version', -]); +export const Device = makeSingleton(DeviceService); // Convert some Angular and Ionic injectables to singletons. export const NgZone = makeSingleton(NgZoneService); export const Http = makeSingleton(HttpClient); -export const Platform = makeSingleton(PlatformService, ['isRTL', 'resume']); +export const Platform = makeSingleton(PlatformService); export const ActionSheetController = makeSingleton(ActionSheetControllerService); export const AlertController = makeSingleton(AlertControllerService); export const LoadingController = makeSingleton(LoadingControllerService); @@ -253,10 +195,10 @@ export const ModalController = makeSingleton(ModalControllerService); export const PopoverController = makeSingleton(PopoverControllerService); export const ToastController = makeSingleton(ToastControllerService); export const GestureController = makeSingleton(GestureControllerService); -export const ApplicationInit = makeSingleton(ApplicationInitStatus, ['donePromise']); +export const ApplicationInit = makeSingleton(ApplicationInitStatus); export const Application = makeSingleton(ApplicationRef); export const NavController = makeSingleton(NavControllerService); -export const Router = makeSingleton(RouterService, ['routerState', 'url']); +export const Router = makeSingleton(RouterService); // Convert external libraries injectables. -export const Translate = makeSingleton(TranslateService, ['onLangChange', 'translations']); +export const Translate = makeSingleton(TranslateService); diff --git a/src/core/singletons/tests/singletons.test.ts b/src/core/singletons/tests/singletons.test.ts index 412a4fe8d..6f6695679 100644 --- a/src/core/singletons/tests/singletons.test.ts +++ b/src/core/singletons/tests/singletons.test.ts @@ -19,12 +19,12 @@ import { MilkyWayService } from './stubs'; describe('Singletons', () => { - let MilkyWay: CoreSingletonProxy; + let MilkyWay: CoreSingletonProxy; beforeEach(() => { setSingletonsInjector(mock({ get: serviceClass => new serviceClass() })); - MilkyWay = makeSingleton(MilkyWayService, ['MEANING_OF_LIFE']); + MilkyWay = makeSingleton(MilkyWayService); }); it('works using the service instance', () => { @@ -35,10 +35,22 @@ describe('Singletons', () => { expect(MilkyWay.getTheMeaningOfLife()).toBe(42); }); + it('works using magic methods defined as getters', () => { + expect(MilkyWay.reduceYears(2)).toBe(-2); + }); + it('works using magic getters', () => { expect(MilkyWay.MEANING_OF_LIFE).toBe(42); }); + it('works using magic getters defined dynamically', () => { + expect(MilkyWay.exists).toBeUndefined(); + + MilkyWay.bigBang(); + + expect(MilkyWay.exists).toBe(true); + }); + it('magic getters use the same instance', () => { expect(MilkyWay.addYears(1)).toBe(1); expect(MilkyWay.instance.addYears(1)).toBe(2); diff --git a/src/core/singletons/tests/stubs.ts b/src/core/singletons/tests/stubs.ts index 7850cf6cd..677bace64 100644 --- a/src/core/singletons/tests/stubs.ts +++ b/src/core/singletons/tests/stubs.ts @@ -22,10 +22,17 @@ export class Galaxy { export class MilkyWayService extends Galaxy { + exists?: boolean; readonly MEANING_OF_LIFE = 42; private years = 0; + reduceYears!: (years: number) => number; + + bigBang(): void { + this.exists = true; + } + getTheMeaningOfLife(): number { return this.MEANING_OF_LIFE; } @@ -37,3 +44,16 @@ export class MilkyWayService extends Galaxy { } } + +Object.defineProperty(MilkyWayService.prototype, 'reduceYears', { + get: () => function(years: number) { + // eslint-disable-next-line no-invalid-this + const self = this as { years: number }; + + self.years -= years; + + return self.years; + }, + enumerable: true, + configurable: true, +}); diff --git a/src/testing/setup.ts b/src/testing/setup.ts index d181c5f93..9972e2e8e 100644 --- a/src/testing/setup.ts +++ b/src/testing/setup.ts @@ -14,7 +14,18 @@ import 'jest-preset-angular'; +import { setCreateSingletonMethodProxy } from '@singletons'; + // eslint-disable-next-line no-console console.debug = () => { // Silence. }; + +// Override the method to create singleton method proxies in order to facilitate setting up +// test expectations about method calls. +setCreateSingletonMethodProxy( + (instance, method, property) => + instance[`mock_${String(property)}`] = + instance[`mock_${String(property)}`] ?? + jest.fn((...args) => method.call(instance, ...args)), +); diff --git a/src/testing/utils.ts b/src/testing/utils.ts index d27ef231c..0bb4e7fff 100644 --- a/src/testing/utils.ts +++ b/src/testing/utils.ts @@ -40,6 +40,16 @@ export function mock( const methods = Array.isArray(methodsOrInstance) ? methodsOrInstance : []; + for (const property of Object.getOwnPropertyNames(instance)) { + const value = instance[property]; + + if (typeof value !== 'function') { + continue; + } + + instance[property] = jest.fn((...args) => value.call(instance, ...args)); + } + for (const method of methods) { instance[method] = jest.fn(); } @@ -66,17 +76,6 @@ export function mockSingleton( singleton.setInstance(mockInstance); - for (const [property, descriptor] of Object.entries(Object.getOwnPropertyDescriptors(singleton))) { - if (typeof descriptor.value !== 'function' || property === 'setInstance') { - continue; - } - - Object.defineProperty(singleton, property, { - value: jest.fn(descriptor.value), - configurable: true, - }); - } - return mockInstance; }