From 3d77846dd7e07d2f5a1a2a84c8ed359ab675a603 Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Fri, 1 Apr 2022 14:25:14 +0200 Subject: [PATCH] refactor(@angular/cli): create a `memoize` decorator With this change we clean up repeated caching code by creating a `memoize` decorator that can be used on get accessors and methods. --- .../architect-command-module.ts | 2 + .../cli/src/command-builder/command-module.ts | 13 +- .../schematics-command-module.ts | 36 +--- packages/angular/cli/src/commands/new/cli.ts | 10 +- packages/angular/cli/src/utilities/memoize.ts | 84 +++++++++ .../angular/cli/src/utilities/memoize_spec.ts | 160 ++++++++++++++++++ .../cli/src/utilities/package-manager.ts | 5 +- tsconfig.json | 1 + 8 files changed, 269 insertions(+), 42 deletions(-) create mode 100644 packages/angular/cli/src/utilities/memoize.ts create mode 100644 packages/angular/cli/src/utilities/memoize_spec.ts diff --git a/packages/angular/cli/src/command-builder/architect-command-module.ts b/packages/angular/cli/src/command-builder/architect-command-module.ts index 0678abf2a5..db6aa449fc 100644 --- a/packages/angular/cli/src/command-builder/architect-command-module.ts +++ b/packages/angular/cli/src/command-builder/architect-command-module.ts @@ -8,6 +8,7 @@ import { Argv } from 'yargs'; import { getProjectByCwd } from '../utilities/config'; +import { memoize } from '../utilities/memoize'; import { ArchitectBaseCommandModule } from './architect-base-command-module'; import { CommandModuleError, @@ -110,6 +111,7 @@ export abstract class ArchitectCommandModule return this.commandName; } + @memoize private getProjectNamesByTarget(target: string): string[] | undefined { const workspace = this.getWorkspaceOrThrow(); diff --git a/packages/angular/cli/src/command-builder/command-module.ts b/packages/angular/cli/src/command-builder/command-module.ts index d513677906..2d9ec61937 100644 --- a/packages/angular/cli/src/command-builder/command-module.ts +++ b/packages/angular/cli/src/command-builder/command-module.ts @@ -20,6 +20,7 @@ import { import { Parser as yargsParser } from 'yargs/helpers'; import { createAnalytics } from '../analytics/analytics'; import { AngularWorkspace } from '../utilities/config'; +import { memoize } from '../utilities/memoize'; import { PackageManagerUtils } from '../utilities/package-manager'; import { Option } from './utilities/json-schema'; @@ -169,17 +170,13 @@ export abstract class CommandModule implements CommandModuleI }); } - private _analytics: analytics.Analytics | undefined; - protected async getAnalytics(): Promise { - if (this._analytics) { - return this._analytics; - } - - return (this._analytics = await createAnalytics( + @memoize + protected getAnalytics(): Promise { + return createAnalytics( !!this.context.workspace, // Don't prompt for `ng update` and `ng analytics` commands. ['update', 'analytics'].includes(this.commandName), - )); + ); } /** diff --git a/packages/angular/cli/src/command-builder/schematics-command-module.ts b/packages/angular/cli/src/command-builder/schematics-command-module.ts index 8129e0b706..986edbdf8a 100644 --- a/packages/angular/cli/src/command-builder/schematics-command-module.ts +++ b/packages/angular/cli/src/command-builder/schematics-command-module.ts @@ -16,6 +16,7 @@ import { import type { CheckboxQuestion, Question } from 'inquirer'; import { Argv } from 'yargs'; import { getProjectByCwd, getSchematicDefaults } from '../utilities/config'; +import { memoize } from '../utilities/memoize'; import { isTTY } from '../utilities/tty'; import { CommandModule, @@ -90,32 +91,19 @@ export abstract class SchematicsCommandModule return parseJsonSchemaToOptions(workflow.registry, schemaJson); } - private _workflowForBuilder = new Map(); + @memoize protected getOrCreateWorkflowForBuilder(collectionName: string): NodeWorkflow { - const cached = this._workflowForBuilder.get(collectionName); - if (cached) { - return cached; - } - - const workflow = new NodeWorkflow(this.context.root, { + return new NodeWorkflow(this.context.root, { resolvePaths: this.getResolvePaths(collectionName), engineHostCreator: (options) => new SchematicEngineHost(options.resolvePaths), }); - - this._workflowForBuilder.set(collectionName, workflow); - - return workflow; } - private _workflowForExecution: NodeWorkflow | undefined; + @memoize protected async getOrCreateWorkflowForExecution( collectionName: string, options: SchematicsExecutionOptions, ): Promise { - if (this._workflowForExecution) { - return this._workflowForExecution; - } - const { logger, root, packageManager } = this.context; const { force, dryRun, packageRegistry } = options; @@ -241,15 +229,11 @@ export abstract class SchematicsCommandModule }); } - return (this._workflowForExecution = workflow); + return workflow; } - private _schematicCollections: Set | undefined; + @memoize protected async getSchematicCollections(): Promise> { - if (this._schematicCollections) { - return this._schematicCollections; - } - const getSchematicCollections = ( configSection: Record | undefined, ): Set | undefined => { @@ -273,8 +257,6 @@ export abstract class SchematicsCommandModule if (project) { const value = getSchematicCollections(workspace.getProjectCli(project)); if (value) { - this._schematicCollections = value; - return value; } } @@ -284,14 +266,10 @@ export abstract class SchematicsCommandModule getSchematicCollections(workspace?.getCli()) ?? getSchematicCollections(globalConfiguration?.getCli()); if (value) { - this._schematicCollections = value; - return value; } - this._schematicCollections = new Set([DEFAULT_SCHEMATICS_COLLECTION]); - - return this._schematicCollections; + return new Set([DEFAULT_SCHEMATICS_COLLECTION]); } protected parseSchematicInfo( diff --git a/packages/angular/cli/src/commands/new/cli.ts b/packages/angular/cli/src/commands/new/cli.ts index 70b0c12e88..d93b701ccd 100644 --- a/packages/angular/cli/src/commands/new/cli.ts +++ b/packages/angular/cli/src/commands/new/cli.ts @@ -63,10 +63,14 @@ export class NewCommandModule async run(options: Options & OtherOptions): Promise { // Register the version of the CLI in the registry. const collectionName = options.collection ?? (await this.getCollectionFromConfig()); - const workflow = await this.getOrCreateWorkflowForExecution(collectionName, options); - workflow.registry.addSmartDefaultProvider('ng-cli-version', () => VERSION.full); - const { dryRun, force, interactive, defaults, collection, ...schematicOptions } = options; + const workflow = await this.getOrCreateWorkflowForExecution(collectionName, { + dryRun, + force, + interactive, + defaults, + }); + workflow.registry.addSmartDefaultProvider('ng-cli-version', () => VERSION.full); // Compatibility check for NPM 7 if ( diff --git a/packages/angular/cli/src/utilities/memoize.ts b/packages/angular/cli/src/utilities/memoize.ts new file mode 100644 index 0000000000..6994dbf5e9 --- /dev/null +++ b/packages/angular/cli/src/utilities/memoize.ts @@ -0,0 +1,84 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +/** + * A decorator that memoizes methods and getters. + * + * **Note**: Be cautious where and how to use this decorator as the size of the cache will grow unbounded. + * + * @see https://en.wikipedia.org/wiki/Memoization + */ +export function memoize( + target: Object, + propertyKey: string | symbol, + descriptor: TypedPropertyDescriptor, +): TypedPropertyDescriptor { + const descriptorPropertyName = descriptor.get ? 'get' : 'value'; + const originalMethod: unknown = descriptor[descriptorPropertyName]; + + if (typeof originalMethod !== 'function') { + throw new Error('Memoize decorator can only be used on methods or get accessors.'); + } + + const cache = new Map(); + + return { + ...descriptor, + [descriptorPropertyName]: function (this: unknown, ...args: unknown[]) { + for (const arg of args) { + if (!isJSONSerializable(arg)) { + throw new Error( + `Argument ${isNonPrimitive(arg) ? arg.toString() : arg} is JSON serializable.`, + ); + } + } + + const key = JSON.stringify(args); + if (cache.has(key)) { + return cache.get(key); + } + + const result = originalMethod.apply(this, args); + cache.set(key, result); + + return result; + }, + }; +} + +/** Method to check if value is a non primitive. */ +function isNonPrimitive(value: unknown): value is object | Function | symbol { + return ( + (value !== null && typeof value === 'object') || + typeof value === 'function' || + typeof value === 'symbol' + ); +} + +/** Method to check if the values are JSON serializable */ +function isJSONSerializable(value: unknown): boolean { + if (!isNonPrimitive(value)) { + // Can be seralized since it's a primitive. + return true; + } + + let nestedValues: unknown[] | undefined; + if (Array.isArray(value)) { + // It's an array, check each item. + nestedValues = value; + } else if (Object.prototype.toString.call(value) === '[object Object]') { + // It's a plain object, check each value. + nestedValues = Object.values(value); + } + + if (!nestedValues || nestedValues.some((v) => !isJSONSerializable(v))) { + return false; + } + + return true; +} diff --git a/packages/angular/cli/src/utilities/memoize_spec.ts b/packages/angular/cli/src/utilities/memoize_spec.ts new file mode 100644 index 0000000000..c1d06fdf4c --- /dev/null +++ b/packages/angular/cli/src/utilities/memoize_spec.ts @@ -0,0 +1,160 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import { memoize } from './memoize'; + +describe('memoize', () => { + class Dummy { + @memoize + get random(): number { + return Math.random(); + } + + @memoize + getRandom(_parameter?: unknown): number { + return Math.random(); + } + + @memoize + async getRandomAsync(): Promise { + return Math.random(); + } + } + + it('should call method once', () => { + const dummy = new Dummy(); + const val1 = dummy.getRandom(); + const val2 = dummy.getRandom(); + + // Should return same value since memoized + expect(val1).toBe(val2); + }); + + it('should call method once (async)', async () => { + const dummy = new Dummy(); + const [val1, val2] = await Promise.all([dummy.getRandomAsync(), dummy.getRandomAsync()]); + + // Should return same value since memoized + expect(val1).toBe(val2); + }); + + it('should call getter once', () => { + const dummy = new Dummy(); + const val1 = dummy.random; + const val2 = dummy.random; + + // Should return same value since memoized + expect(val2).toBe(val1); + }); + + it('should call method when parameter changes', () => { + const dummy = new Dummy(); + const val1 = dummy.getRandom(1); + const val2 = dummy.getRandom(2); + const val3 = dummy.getRandom(1); + const val4 = dummy.getRandom(2); + + // Should return same value since memoized + expect(val1).not.toBe(val2); + expect(val1).toBe(val3); + expect(val2).toBe(val4); + }); + + it('should error when used on non getters and methods', () => { + const test = () => { + class DummyError { + @memoize + set random(_value: number) {} + } + + return new DummyError(); + }; + + expect(test).toThrowError('Memoize decorator can only be used on methods or get accessors.'); + }); + + describe('validate method arguments', () => { + it('should error when using Map', () => { + const test = () => new Dummy().getRandom(new Map()); + + expect(test).toThrowError(/Argument \[object Map\] is JSON serializable./); + }); + + it('should error when using Symbol', () => { + const test = () => new Dummy().getRandom(Symbol('')); + + expect(test).toThrowError(/Argument Symbol\(\) is JSON serializable/); + }); + + it('should error when using Function', () => { + const test = () => new Dummy().getRandom(function () {}); + + expect(test).toThrowError(/Argument function \(\) { } is JSON serializable/); + }); + + it('should error when using Map in an array', () => { + const test = () => new Dummy().getRandom([new Map(), true]); + + expect(test).toThrowError(/Argument \[object Map\],true is JSON serializable/); + }); + + it('should error when using Map in an Object', () => { + const test = () => new Dummy().getRandom({ foo: true, prop: new Map() }); + + expect(test).toThrowError(/Argument \[object Object\] is JSON serializable/); + }); + + it('should error when using Function in an Object', () => { + const test = () => new Dummy().getRandom({ foo: true, prop: function () {} }); + + expect(test).toThrowError(/Argument \[object Object\] is JSON serializable/); + }); + + it('should not error when using primitive values in an array', () => { + const test = () => new Dummy().getRandom([1, true, ['foo']]); + + expect(test).not.toThrow(); + }); + + it('should not error when using primitive values in an Object', () => { + const test = () => new Dummy().getRandom({ foo: true, prop: [1, true] }); + + expect(test).not.toThrow(); + }); + + it('should not error when using Boolean', () => { + const test = () => new Dummy().getRandom(true); + + expect(test).not.toThrow(); + }); + + it('should not error when using String', () => { + const test = () => new Dummy().getRandom('foo'); + + expect(test).not.toThrow(); + }); + + it('should not error when using Number', () => { + const test = () => new Dummy().getRandom(1); + + expect(test).not.toThrow(); + }); + + it('should not error when using null', () => { + const test = () => new Dummy().getRandom(null); + + expect(test).not.toThrow(); + }); + + it('should not error when using undefined', () => { + const test = () => new Dummy().getRandom(undefined); + + expect(test).not.toThrow(); + }); + }); +}); diff --git a/packages/angular/cli/src/utilities/package-manager.ts b/packages/angular/cli/src/utilities/package-manager.ts index a2b8096d5e..fddcfb2f86 100644 --- a/packages/angular/cli/src/utilities/package-manager.ts +++ b/packages/angular/cli/src/utilities/package-manager.ts @@ -14,6 +14,7 @@ import { join } from 'path'; import { satisfies, valid } from 'semver'; import { PackageManager } from '../../lib/config/workspace-schema'; import { AngularWorkspace, getProjectByCwd } from './config'; +import { memoize } from './memoize'; import { Spinner } from './spinner'; interface PackageManagerOptions { @@ -218,7 +219,7 @@ export class PackageManagerUtils { }); } - // TODO(alan-agius4): use the memoize decorator when it's merged. + @memoize private getVersion(name: PackageManager): string | undefined { try { return execSync(`${name} --version`, { @@ -236,7 +237,7 @@ export class PackageManagerUtils { } } - // TODO(alan-agius4): use the memoize decorator when it's merged. + @memoize private getName(): PackageManager { const packageManager = this.getConfiguredPackageManager(); if (packageManager) { diff --git a/tsconfig.json b/tsconfig.json index 222e9038cc..3c19b0e22c 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -5,6 +5,7 @@ "module": "commonjs", "moduleResolution": "node", "noEmitOnError": true, + "experimentalDecorators": true, "noFallthroughCasesInSwitch": true, "noImplicitOverride": true, "noUnusedParameters": false,