From 94c3c855d05bbbc2ec2f9cb549c0d35c5cb36363 Mon Sep 17 00:00:00 2001 From: matt Date: Mon, 13 Jan 2020 15:10:59 -0800 Subject: [PATCH 1/6] feat: adds MultiMethodDecoratorFactory --- .vscode/launch.json | 16 ++ packages/build/bin/get-dist-file.js | 75 +++++++ packages/metadata/README.md | 86 ++++++++ .../__tests__/unit/decorator-factory.unit.ts | 188 ++++++++++++++++++ packages/metadata/src/decorator-factory.ts | 75 +++++++ 5 files changed, 440 insertions(+) create mode 100644 packages/build/bin/get-dist-file.js diff --git a/.vscode/launch.json b/.vscode/launch.json index c6adda413542..7715a6bc9e35 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -18,6 +18,22 @@ "0" ] }, + { + "type": "node", + "request": "launch", + "name": "Debug Current Test File", + "program": "${workspaceRoot}/packages/build/node_modules/.bin/_mocha", + "cwd": "${workspaceRoot}", + "autoAttachChildProcesses": true, + "args": [ + "--config", + "${workspaceRoot}/packages/build/config/.mocharc.json", + "-t", + "0", + "$(node ${workspaceRoot}/packages/build/bin/get-dist-file ${file})" + ], + "disableOptimisticBPs": true + }, { "type": "node", "request": "attach", diff --git a/packages/build/bin/get-dist-file.js b/packages/build/bin/get-dist-file.js new file mode 100644 index 000000000000..418c4a408fff --- /dev/null +++ b/packages/build/bin/get-dist-file.js @@ -0,0 +1,75 @@ +#!/usr/bin/env node +// Copyright IBM Corp. 2018. All Rights Reserved. +// Node module: @loopback/build +// This file is licensed under the MIT License. +// License text available at https://opensource.org/licenses/MIT + +/* +======== +This is used in the launch.json to enable you to debug a test file written in +typescript. This function attempts to convert the passed typescript file to +the best-gust output javascript file. + +It walks up the filesystem from the current file, stops at package.json, and +looks in `dist` + +Ideally, we could use the typescript compiler and tsconfig.json to get the +explicit output file instead of trying to guess it. + +Ex: +```jsonc +{ + "version": "0.2.0", + "configurations": [ + { + "type": "node", + "request": "launch", + "name": "Debug Current Test File", + "program": "${workspaceRoot}/packages/build/node_modules/.bin/_mocha", + "cwd": "${workspaceRoot}", + "autoAttachChildProcesses": true, + "args": [ + "--config", + "${workspaceRoot}/packages/build/config/.mocharc.json", + "-t", + "0", + "$(node ${workspaceRoot}/packages/build/bin/get-dist-file ${file})" + ], + "disableOptimisticBPs": true + } + ] +} +``` + +For your personal projects, you can sub directlry from loopback: +``` +"$(node ${workspaceRoot}/node_modules/@loopback/build/bin/get-dist-file ${file})" +``` +You still have to compile the package/project first. +======== +*/ + +'use strict'; +const path = require('path'); +const fs = require('fs'); + +function findDistFile(filename) { + const absolutePath = path.resolve(filename); + let currentDir = path.dirname(absolutePath); + let isPackageRoot = fs.existsSync(path.resolve(currentDir, 'package.json')); + while (!isPackageRoot) { + currentDir = path.join(currentDir, '..'); + isPackageRoot = fs.existsSync(path.resolve(currentDir, 'package.json')); + } + const base = path.resolve(currentDir); + const relative = path.relative(currentDir, absolutePath); + const resultPath = relative + .replace(/^src\//, 'dist/') + .replace(/\.ts$/, '.js'); + return path.resolve(base, resultPath); +} + +module.exports = findDistFile; +if (require.main === module) { + process.stdout.write(findDistFile(process.argv.splice(-1)[0])); +} diff --git a/packages/metadata/README.md b/packages/metadata/README.md index e1537be5b6dd..4d23763a26ab 100644 --- a/packages/metadata/README.md +++ b/packages/metadata/README.md @@ -87,6 +87,92 @@ class MyController { } ``` +### To create a decorator that can be used multiple times on a single method + +Instead of a single immutable object to be merged, the +`MethodMultiDecoratorFactory` reduced parameters into a flat array of items. +When fetching the metadata later, you will receive it as an array. + +```ts +import {MethodMultiDecoratorFactory} from '@loopback/metadata'; + +function myMultiMethodDecorator(spec: object): MethodDecorator { + return MethodMultiDecoratorFactory.createDecorator( + 'metadata-key-for-my-method-multi-decorator', + spec, + ); +} +``` + +Now, you can use it multiple times on a method: + +```ts +class MyController { + @myMultiMethodDecorator({x: 1}) + @myMultiMethodDecorator({y: 2}) + @myMultiMethodDecorator({z: 3}) + public point() {} +} + +class MyOtherController { + @myMultiMethodDecorator([{x: 1}, {y: 2}, {z: 3}]) + public point() {} +} +``` + +And when you access this data: + +```ts +const arrayOfSpecs = MetadataInspector.getMethodMetadata( + 'metadata-key-for-my-method-multi-decorator', + constructor.prototype, + op, +); + +// [{x:1}, {y:2}, {z: 3}] +``` + +Note that the order of items is **not** guaranteed and should be treated as +unsorted. + +You can also create a decorator that takes an object that can contain an array: + +```ts +interface Point { + x?: number; + y?: number; + z?: number; +} +interface GeometryMetadata { + points: Point[]; +} +function geometry(points: Point | Point[]): MethodDecorator { + return MethodMultiDecoratorFactory.createDecorator( + 'metadata-key-for-my-method-multi-decorator', + points: Array.isArray(points) ? points : [points], + ); +} + +class MyGeoController { + @geometry({x: 1}) + @geometry([{x:2}, {y:3}]) + @geometry({z: 5}) + public abstract() {} +} + +const arrayOfSpecs = MetadataInspector.getMethodMetadata( + 'metadata-key-for-my-method-multi-decorator', + constructor.prototype, + op, +); + +// [ +// { points: [{x: 1}]}, +// { points: [{x:2}, {y:3}]}, +// { points: [{z: 5}]}, +// ] +``` + ### To create a property decorator ```ts diff --git a/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts b/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts index d942761399eb..a912d9c7f378 100644 --- a/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts +++ b/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts @@ -8,6 +8,7 @@ import { ClassDecoratorFactory, DecoratorFactory, MethodDecoratorFactory, + MethodMultiDecoratorFactory, MethodParameterDecoratorFactory, ParameterDecoratorFactory, PropertyDecoratorFactory, @@ -523,6 +524,193 @@ describe('MethodDecoratorFactory for static methods', () => { }); }); +describe('MethodMultiDecoratorFactory', () => { + function methodMultiArrayDecorator(spec: object | object[]): MethodDecorator { + if (Array.isArray(spec)) { + return MethodMultiDecoratorFactory.createDecorator('test', spec); + } else { + return MethodMultiDecoratorFactory.createDecorator('test', [spec]); + } + } + + function methodMultiDecorator(spec: object): MethodDecorator { + return MethodMultiDecoratorFactory.createDecorator('test', spec); + } + + class BaseController { + @methodMultiArrayDecorator({x: 1}) + public myMethod() {} + + @methodMultiArrayDecorator({foo: 1}) + @methodMultiArrayDecorator({foo: 2}) + @methodMultiArrayDecorator([{foo: 3}, {foo: 4}]) + public multiMethod() {} + + @methodMultiDecorator({a: 'a'}) + @methodMultiDecorator({b: 'b'}) + public checkDecorator() {} + } + + class SubController extends BaseController { + @methodMultiArrayDecorator({y: 2}) + public myMethod() {} + + @methodMultiArrayDecorator({bar: 1}) + @methodMultiArrayDecorator([{bar: 2}, {bar: 3}]) + public multiMethod() {} + } + + describe('single-decorator methods', () => { + it('applies metadata to a method', () => { + const meta = Reflector.getOwnMetadata('test', BaseController.prototype); + expect(meta.myMethod).to.eql([{x: 1}]); + }); + + it('merges with base method metadata', () => { + const meta = Reflector.getOwnMetadata('test', SubController.prototype); + expect(meta.myMethod).to.eql([{x: 1}, {y: 2}]); + }); + + it('does not mutate base method metadata', () => { + const meta = Reflector.getOwnMetadata('test', BaseController.prototype); + expect(meta.myMethod).to.eql([{x: 1}]); + }); + }); + + describe('multi-decorator methods', () => { + it('applies to non-array decorator creation', () => { + const meta = Reflector.getOwnMetadata('test', BaseController.prototype); + expect(meta.checkDecorator).to.containDeep([{a: 'a'}, {b: 'b'}]); + }); + + it('applies metadata to a method', () => { + const meta = Reflector.getOwnMetadata('test', BaseController.prototype); + expect(meta.multiMethod).to.containDeep([ + {foo: 4}, + {foo: 3}, + {foo: 2}, + {foo: 1}, + ]); + }); + + it('merges with base method metadata', () => { + const meta = Reflector.getOwnMetadata('test', SubController.prototype); + expect(meta.multiMethod).to.containDeep([ + {foo: 4}, + {foo: 3}, + {foo: 2}, + {foo: 1}, + {bar: 3}, + {bar: 2}, + {bar: 1}, + ]); + }); + + it('does not mutate base method metadata', () => { + const meta = Reflector.getOwnMetadata('test', BaseController.prototype); + expect(meta.multiMethod).to.containDeep([ + {foo: 1}, + {foo: 2}, + {foo: 3}, + {foo: 4}, + ]); + }); + }); +}); +describe('MethodMultiDecoratorFactory for static methods', () => { + function methodMultiArrayDecorator(spec: object | object[]): MethodDecorator { + if (Array.isArray(spec)) { + return MethodMultiDecoratorFactory.createDecorator('test', spec); + } else { + return MethodMultiDecoratorFactory.createDecorator('test', [spec]); + } + } + + function methodMultiDecorator(spec: object): MethodDecorator { + return MethodMultiDecoratorFactory.createDecorator('test', spec); + } + + class BaseController { + @methodMultiArrayDecorator({x: 1}) + static myMethod() {} + + @methodMultiArrayDecorator({foo: 1}) + @methodMultiArrayDecorator({foo: 2}) + @methodMultiArrayDecorator([{foo: 3}, {foo: 4}]) + static multiMethod() {} + + @methodMultiDecorator({a: 'a'}) + @methodMultiDecorator({b: 'b'}) + static checkDecorator() {} + } + + class SubController extends BaseController { + @methodMultiArrayDecorator({y: 2}) + static myMethod() {} + + @methodMultiArrayDecorator({bar: 1}) + @methodMultiArrayDecorator([{bar: 2}, {bar: 3}]) + static multiMethod() {} + } + + describe('single-decorator methods', () => { + it('applies metadata to a method', () => { + const meta = Reflector.getOwnMetadata('test', BaseController); + expect(meta.myMethod).to.eql([{x: 1}]); + }); + + it('merges with base method metadata', () => { + const meta = Reflector.getOwnMetadata('test', SubController); + expect(meta.myMethod).to.eql([{x: 1}, {y: 2}]); + }); + + it('does not mutate base method metadata', () => { + const meta = Reflector.getOwnMetadata('test', BaseController); + expect(meta.myMethod).to.eql([{x: 1}]); + }); + }); + + describe('multi-decorator methods', () => { + it('applies metadata to a method', () => { + const meta = Reflector.getOwnMetadata('test', BaseController); + expect(meta.multiMethod).to.containDeep([ + {foo: 4}, + {foo: 3}, + {foo: 2}, + {foo: 1}, + ]); + }); + + it('applies to non-array decorator creation', () => { + const meta = Reflector.getOwnMetadata('test', BaseController); + expect(meta.checkDecorator).to.containDeep([{a: 'a'}, {b: 'b'}]); + }); + + it('merges with base method metadata', () => { + const meta = Reflector.getOwnMetadata('test', SubController); + expect(meta.multiMethod).to.containDeep([ + {foo: 4}, + {foo: 3}, + {foo: 2}, + {foo: 1}, + {bar: 3}, + {bar: 2}, + {bar: 1}, + ]); + }); + + it('does not mutate base method metadata', () => { + const meta = Reflector.getOwnMetadata('test', BaseController); + expect(meta.multiMethod).to.containDeep([ + {foo: 1}, + {foo: 2}, + {foo: 3}, + {foo: 4}, + ]); + }); + }); +}); + describe('ParameterDecoratorFactory', () => { /** * Define `@parameterDecorator(spec)` diff --git a/packages/metadata/src/decorator-factory.ts b/packages/metadata/src/decorator-factory.ts index 3c8a10dbbc14..10888c621812 100644 --- a/packages/metadata/src/decorator-factory.ts +++ b/packages/metadata/src/decorator-factory.ts @@ -789,3 +789,78 @@ export class MethodParameterDecoratorFactory extends DecoratorFactory< ); } } + +/** + * Factory for an append-array of method-level decorators + * The @response metadata for a method is an array. + * Each item in the array should be a single value, containing + * a response code and a single spec or Model. This should allow: + * ```ts + * @response(200, MyFirstModel) + * @response(403, [NotAuthorizedReasonOne, NotAuthorizedReasonTwo]) + * @response(404, NotFoundOne) + * @response(404, NotFoundTwo) + * @response(409, {schema: {}}) + * public async myMethod() {} + * ``` + * + * In the case that a ResponseObject is passed, it becomes the + * default for description/content, and if possible, further Models are + * incorporated as a `oneOf: []` array. + * + * In the case that a ReferenceObject is passed, it and it alone is used, since + * references can be external and we cannot `oneOf` their content. + * + * The factory creates and updates an array of items T[], and the getter + * provides the values as that array. + */ +export class MethodMultiDecoratorFactory extends MethodDecoratorFactory< + T[] +> { + protected mergeWithInherited( + inheritedMetadata: MetadataMap, + target: Object, + methodName?: string, + ) { + inheritedMetadata = inheritedMetadata || {}; + + inheritedMetadata[methodName!] = this._mergeArray( + inheritedMetadata[methodName!], + this.withTarget(this.spec, target), + ); + + return inheritedMetadata; + } + + protected mergeWithOwn( + ownMetadata: MetadataMap, + target: Object, + methodName?: string, + methodDescriptor?: TypedPropertyDescriptor | number, + ) { + ownMetadata = ownMetadata || {}; + ownMetadata[methodName!] = this._mergeArray( + ownMetadata[methodName!], + this.withTarget(this.spec, target), + ); + return ownMetadata; + } + + private _mergeArray(result: T[], methodMeta: T | T[]) { + if (!result) { + if (Array.isArray(methodMeta)) { + result = methodMeta; + } else { + result = [methodMeta]; + } + } else { + if (Array.isArray(methodMeta)) { + result.push(...methodMeta); + } else { + result.push(methodMeta); + } + } + + return result; + } +} From 1b2d5456d1ad720a7e824e19bb9905ad7528f854 Mon Sep 17 00:00:00 2001 From: matt Date: Mon, 13 Jan 2020 16:08:38 -0800 Subject: [PATCH 2/6] fix: typos --- packages/build/bin/get-dist-file.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/build/bin/get-dist-file.js b/packages/build/bin/get-dist-file.js index 418c4a408fff..4f0e1666d352 100644 --- a/packages/build/bin/get-dist-file.js +++ b/packages/build/bin/get-dist-file.js @@ -8,13 +8,14 @@ ======== This is used in the launch.json to enable you to debug a test file written in typescript. This function attempts to convert the passed typescript file to -the best-gust output javascript file. +the best-guess output javascript file. It walks up the filesystem from the current file, stops at package.json, and looks in `dist` -Ideally, we could use the typescript compiler and tsconfig.json to get the -explicit output file instead of trying to guess it. +Ideally, we could somehow use the typescript compiler and tsconfig.json to get +the explicit output file instead of trying to guess it, but there might be +overhead. Ex: ```jsonc @@ -41,7 +42,7 @@ Ex: } ``` -For your personal projects, you can sub directlry from loopback: +For your personal projects, you can invoke directly from @loopback/build: ``` "$(node ${workspaceRoot}/node_modules/@loopback/build/bin/get-dist-file ${file})" ``` From 05369d0a8021021fcf9915f9917cc410957f54ae Mon Sep 17 00:00:00 2001 From: matt Date: Tue, 14 Jan 2020 10:14:21 -0800 Subject: [PATCH 3/6] fix: remove updated debug config for PR --- .vscode/launch.json | 16 ------ packages/build/bin/get-dist-file.js | 76 ----------------------------- 2 files changed, 92 deletions(-) delete mode 100644 packages/build/bin/get-dist-file.js diff --git a/.vscode/launch.json b/.vscode/launch.json index 7715a6bc9e35..c6adda413542 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -18,22 +18,6 @@ "0" ] }, - { - "type": "node", - "request": "launch", - "name": "Debug Current Test File", - "program": "${workspaceRoot}/packages/build/node_modules/.bin/_mocha", - "cwd": "${workspaceRoot}", - "autoAttachChildProcesses": true, - "args": [ - "--config", - "${workspaceRoot}/packages/build/config/.mocharc.json", - "-t", - "0", - "$(node ${workspaceRoot}/packages/build/bin/get-dist-file ${file})" - ], - "disableOptimisticBPs": true - }, { "type": "node", "request": "attach", diff --git a/packages/build/bin/get-dist-file.js b/packages/build/bin/get-dist-file.js deleted file mode 100644 index 4f0e1666d352..000000000000 --- a/packages/build/bin/get-dist-file.js +++ /dev/null @@ -1,76 +0,0 @@ -#!/usr/bin/env node -// Copyright IBM Corp. 2018. All Rights Reserved. -// Node module: @loopback/build -// This file is licensed under the MIT License. -// License text available at https://opensource.org/licenses/MIT - -/* -======== -This is used in the launch.json to enable you to debug a test file written in -typescript. This function attempts to convert the passed typescript file to -the best-guess output javascript file. - -It walks up the filesystem from the current file, stops at package.json, and -looks in `dist` - -Ideally, we could somehow use the typescript compiler and tsconfig.json to get -the explicit output file instead of trying to guess it, but there might be -overhead. - -Ex: -```jsonc -{ - "version": "0.2.0", - "configurations": [ - { - "type": "node", - "request": "launch", - "name": "Debug Current Test File", - "program": "${workspaceRoot}/packages/build/node_modules/.bin/_mocha", - "cwd": "${workspaceRoot}", - "autoAttachChildProcesses": true, - "args": [ - "--config", - "${workspaceRoot}/packages/build/config/.mocharc.json", - "-t", - "0", - "$(node ${workspaceRoot}/packages/build/bin/get-dist-file ${file})" - ], - "disableOptimisticBPs": true - } - ] -} -``` - -For your personal projects, you can invoke directly from @loopback/build: -``` -"$(node ${workspaceRoot}/node_modules/@loopback/build/bin/get-dist-file ${file})" -``` -You still have to compile the package/project first. -======== -*/ - -'use strict'; -const path = require('path'); -const fs = require('fs'); - -function findDistFile(filename) { - const absolutePath = path.resolve(filename); - let currentDir = path.dirname(absolutePath); - let isPackageRoot = fs.existsSync(path.resolve(currentDir, 'package.json')); - while (!isPackageRoot) { - currentDir = path.join(currentDir, '..'); - isPackageRoot = fs.existsSync(path.resolve(currentDir, 'package.json')); - } - const base = path.resolve(currentDir); - const relative = path.relative(currentDir, absolutePath); - const resultPath = relative - .replace(/^src\//, 'dist/') - .replace(/\.ts$/, '.js'); - return path.resolve(base, resultPath); -} - -module.exports = findDistFile; -if (require.main === module) { - process.stdout.write(findDistFile(process.argv.splice(-1)[0])); -} From e7ed3670b8f519d98806d630bbd917def128c9d9 Mon Sep 17 00:00:00 2001 From: matt Date: Tue, 14 Jan 2020 13:35:19 -0800 Subject: [PATCH 4/6] fix: clarify the decorator application order --- packages/metadata/README.md | 33 +++++++++++++---- .../__tests__/unit/decorator-factory.unit.ts | 36 +++++-------------- packages/metadata/src/decorator-factory.ts | 4 +-- 3 files changed, 37 insertions(+), 36 deletions(-) diff --git a/packages/metadata/README.md b/packages/metadata/README.md index 4d23763a26ab..84b07ff1beb7 100644 --- a/packages/metadata/README.md +++ b/packages/metadata/README.md @@ -129,11 +129,32 @@ const arrayOfSpecs = MetadataInspector.getMethodMetadata( op, ); -// [{x:1}, {y:2}, {z: 3}] +// [{z: 3}, {y: 2}, {x: 1}] ``` -Note that the order of items is **not** guaranteed and should be treated as -unsorted. +Typescript applies decorators in reverse order per class, from the parent down. +In cases when an array is passed as metadata, it will be reversed to respect +this order. + +```ts +class Parent { + @myMultiMethodDecorator('A') // second + @myMultiMethodDecorator('B') // first + public greet() {} +} + +class Child extends Parent { + @myMultiMethodDecorator(['C', 'D']) // [fourth, third] + public greet() {} +} + +class Grandchild extends Child { + @myMultiMethodDecorator('E') // sixth + @myMultiMethodDecorator('F') // fifth + public greet() {} +} +// getMethodMetadata = ['B', 'A', 'D', 'C', 'F', 'E'] +``` You can also create a decorator that takes an object that can contain an array: @@ -146,16 +167,16 @@ interface Point { interface GeometryMetadata { points: Point[]; } -function geometry(points: Point | Point[]): MethodDecorator { +function geometry(...points: Point[]): MethodDecorator { return MethodMultiDecoratorFactory.createDecorator( 'metadata-key-for-my-method-multi-decorator', - points: Array.isArray(points) ? points : [points], + points, ); } class MyGeoController { @geometry({x: 1}) - @geometry([{x:2}, {y:3}]) + @geometry([{x: 2}, {y: 3}]) @geometry({z: 5}) public abstract() {} } diff --git a/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts b/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts index a912d9c7f378..24f0725c5558 100644 --- a/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts +++ b/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts @@ -580,22 +580,17 @@ describe('MethodMultiDecoratorFactory', () => { describe('multi-decorator methods', () => { it('applies to non-array decorator creation', () => { const meta = Reflector.getOwnMetadata('test', BaseController.prototype); - expect(meta.checkDecorator).to.containDeep([{a: 'a'}, {b: 'b'}]); + expect(meta.checkDecorator).to.eql([{b: 'b'}, {a: 'a'}]); }); it('applies metadata to a method', () => { const meta = Reflector.getOwnMetadata('test', BaseController.prototype); - expect(meta.multiMethod).to.containDeep([ - {foo: 4}, - {foo: 3}, - {foo: 2}, - {foo: 1}, - ]); + expect(meta.multiMethod).to.eql([{foo: 4}, {foo: 3}, {foo: 2}, {foo: 1}]); }); it('merges with base method metadata', () => { const meta = Reflector.getOwnMetadata('test', SubController.prototype); - expect(meta.multiMethod).to.containDeep([ + expect(meta.multiMethod).to.eql([ {foo: 4}, {foo: 3}, {foo: 2}, @@ -608,12 +603,7 @@ describe('MethodMultiDecoratorFactory', () => { it('does not mutate base method metadata', () => { const meta = Reflector.getOwnMetadata('test', BaseController.prototype); - expect(meta.multiMethod).to.containDeep([ - {foo: 1}, - {foo: 2}, - {foo: 3}, - {foo: 4}, - ]); + expect(meta.multiMethod).to.eql([{foo: 4}, {foo: 3}, {foo: 2}, {foo: 1}]); }); }); }); @@ -673,22 +663,17 @@ describe('MethodMultiDecoratorFactory for static methods', () => { describe('multi-decorator methods', () => { it('applies metadata to a method', () => { const meta = Reflector.getOwnMetadata('test', BaseController); - expect(meta.multiMethod).to.containDeep([ - {foo: 4}, - {foo: 3}, - {foo: 2}, - {foo: 1}, - ]); + expect(meta.multiMethod).to.eql([{foo: 4}, {foo: 3}, {foo: 2}, {foo: 1}]); }); it('applies to non-array decorator creation', () => { const meta = Reflector.getOwnMetadata('test', BaseController); - expect(meta.checkDecorator).to.containDeep([{a: 'a'}, {b: 'b'}]); + expect(meta.checkDecorator).to.eql([{b: 'b'}, {a: 'a'}]); }); it('merges with base method metadata', () => { const meta = Reflector.getOwnMetadata('test', SubController); - expect(meta.multiMethod).to.containDeep([ + expect(meta.multiMethod).to.eql([ {foo: 4}, {foo: 3}, {foo: 2}, @@ -701,12 +686,7 @@ describe('MethodMultiDecoratorFactory for static methods', () => { it('does not mutate base method metadata', () => { const meta = Reflector.getOwnMetadata('test', BaseController); - expect(meta.multiMethod).to.containDeep([ - {foo: 1}, - {foo: 2}, - {foo: 3}, - {foo: 4}, - ]); + expect(meta.multiMethod).to.eql([{foo: 4}, {foo: 3}, {foo: 2}, {foo: 1}]); }); }); }); diff --git a/packages/metadata/src/decorator-factory.ts b/packages/metadata/src/decorator-factory.ts index 10888c621812..5aa122d6e1a7 100644 --- a/packages/metadata/src/decorator-factory.ts +++ b/packages/metadata/src/decorator-factory.ts @@ -849,13 +849,13 @@ export class MethodMultiDecoratorFactory extends MethodDecoratorFactory< private _mergeArray(result: T[], methodMeta: T | T[]) { if (!result) { if (Array.isArray(methodMeta)) { - result = methodMeta; + result = methodMeta.reverse(); } else { result = [methodMeta]; } } else { if (Array.isArray(methodMeta)) { - result.push(...methodMeta); + result.push(...methodMeta.reverse()); } else { result.push(methodMeta); } From a9f55d867d2e04656ceec234a8383773bf8d9348 Mon Sep 17 00:00:00 2001 From: matt Date: Tue, 14 Jan 2020 15:06:45 -0800 Subject: [PATCH 5/6] fix: makes multi-decorator behavior more predictable --- packages/metadata/README.md | 11 ++++++----- .../src/__tests__/unit/decorator-factory.unit.ts | 16 ++++++++-------- packages/metadata/src/decorator-factory.ts | 4 ++-- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/packages/metadata/README.md b/packages/metadata/README.md index 84b07ff1beb7..41dff4952083 100644 --- a/packages/metadata/README.md +++ b/packages/metadata/README.md @@ -132,9 +132,10 @@ const arrayOfSpecs = MetadataInspector.getMethodMetadata( // [{z: 3}, {y: 2}, {x: 1}] ``` -Typescript applies decorators in reverse order per class, from the parent down. -In cases when an array is passed as metadata, it will be reversed to respect -this order. +Typescript +[applies decorators in reverse order](https://www.typescriptlang.org/docs/handbook/decorators.html) +per class, from the parent down. The metadata array resurned by `getOwnMetadata` +will be in this order: ```ts class Parent { @@ -144,7 +145,7 @@ class Parent { } class Child extends Parent { - @myMultiMethodDecorator(['C', 'D']) // [fourth, third] + @myMultiMethodDecorator(['C', 'D']) // [third, fourth] public greet() {} } @@ -153,7 +154,7 @@ class Grandchild extends Child { @myMultiMethodDecorator('F') // fifth public greet() {} } -// getMethodMetadata = ['B', 'A', 'D', 'C', 'F', 'E'] +// getMethodMetadata = ['B', 'A', 'C', 'D', 'F', 'E'] ``` You can also create a decorator that takes an object that can contain an array: diff --git a/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts b/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts index 24f0725c5558..a615793953ba 100644 --- a/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts +++ b/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts @@ -585,25 +585,25 @@ describe('MethodMultiDecoratorFactory', () => { it('applies metadata to a method', () => { const meta = Reflector.getOwnMetadata('test', BaseController.prototype); - expect(meta.multiMethod).to.eql([{foo: 4}, {foo: 3}, {foo: 2}, {foo: 1}]); + expect(meta.multiMethod).to.eql([{foo: 3}, {foo: 4}, {foo: 2}, {foo: 1}]); }); it('merges with base method metadata', () => { const meta = Reflector.getOwnMetadata('test', SubController.prototype); expect(meta.multiMethod).to.eql([ - {foo: 4}, {foo: 3}, + {foo: 4}, {foo: 2}, {foo: 1}, - {bar: 3}, {bar: 2}, + {bar: 3}, {bar: 1}, ]); }); it('does not mutate base method metadata', () => { const meta = Reflector.getOwnMetadata('test', BaseController.prototype); - expect(meta.multiMethod).to.eql([{foo: 4}, {foo: 3}, {foo: 2}, {foo: 1}]); + expect(meta.multiMethod).to.eql([{foo: 3}, {foo: 4}, {foo: 2}, {foo: 1}]); }); }); }); @@ -663,7 +663,7 @@ describe('MethodMultiDecoratorFactory for static methods', () => { describe('multi-decorator methods', () => { it('applies metadata to a method', () => { const meta = Reflector.getOwnMetadata('test', BaseController); - expect(meta.multiMethod).to.eql([{foo: 4}, {foo: 3}, {foo: 2}, {foo: 1}]); + expect(meta.multiMethod).to.eql([{foo: 3}, {foo: 4}, {foo: 2}, {foo: 1}]); }); it('applies to non-array decorator creation', () => { @@ -674,19 +674,19 @@ describe('MethodMultiDecoratorFactory for static methods', () => { it('merges with base method metadata', () => { const meta = Reflector.getOwnMetadata('test', SubController); expect(meta.multiMethod).to.eql([ - {foo: 4}, {foo: 3}, + {foo: 4}, {foo: 2}, {foo: 1}, - {bar: 3}, {bar: 2}, + {bar: 3}, {bar: 1}, ]); }); it('does not mutate base method metadata', () => { const meta = Reflector.getOwnMetadata('test', BaseController); - expect(meta.multiMethod).to.eql([{foo: 4}, {foo: 3}, {foo: 2}, {foo: 1}]); + expect(meta.multiMethod).to.eql([{foo: 3}, {foo: 4}, {foo: 2}, {foo: 1}]); }); }); }); diff --git a/packages/metadata/src/decorator-factory.ts b/packages/metadata/src/decorator-factory.ts index 5aa122d6e1a7..10888c621812 100644 --- a/packages/metadata/src/decorator-factory.ts +++ b/packages/metadata/src/decorator-factory.ts @@ -849,13 +849,13 @@ export class MethodMultiDecoratorFactory extends MethodDecoratorFactory< private _mergeArray(result: T[], methodMeta: T | T[]) { if (!result) { if (Array.isArray(methodMeta)) { - result = methodMeta.reverse(); + result = methodMeta; } else { result = [methodMeta]; } } else { if (Array.isArray(methodMeta)) { - result.push(...methodMeta.reverse()); + result.push(...methodMeta); } else { result.push(methodMeta); } From e9599d7fc151bdb58a1ad706ff21cf549880856e Mon Sep 17 00:00:00 2001 From: matt Date: Tue, 14 Jan 2020 15:09:15 -0800 Subject: [PATCH 6/6] fix: ensure tests follow parameter patterns --- packages/metadata/README.md | 2 +- .../__tests__/unit/decorator-factory.unit.ts | 24 +++++++------------ 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/packages/metadata/README.md b/packages/metadata/README.md index 41dff4952083..5b939745f2dc 100644 --- a/packages/metadata/README.md +++ b/packages/metadata/README.md @@ -177,7 +177,7 @@ function geometry(...points: Point[]): MethodDecorator { class MyGeoController { @geometry({x: 1}) - @geometry([{x: 2}, {y: 3}]) + @geometry({x: 2}, {y: 3}) @geometry({z: 5}) public abstract() {} } diff --git a/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts b/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts index a615793953ba..14c613f03596 100644 --- a/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts +++ b/packages/metadata/src/__tests__/unit/decorator-factory.unit.ts @@ -525,12 +525,8 @@ describe('MethodDecoratorFactory for static methods', () => { }); describe('MethodMultiDecoratorFactory', () => { - function methodMultiArrayDecorator(spec: object | object[]): MethodDecorator { - if (Array.isArray(spec)) { - return MethodMultiDecoratorFactory.createDecorator('test', spec); - } else { - return MethodMultiDecoratorFactory.createDecorator('test', [spec]); - } + function methodMultiArrayDecorator(...spec: object[]): MethodDecorator { + return MethodMultiDecoratorFactory.createDecorator('test', spec); } function methodMultiDecorator(spec: object): MethodDecorator { @@ -543,7 +539,7 @@ describe('MethodMultiDecoratorFactory', () => { @methodMultiArrayDecorator({foo: 1}) @methodMultiArrayDecorator({foo: 2}) - @methodMultiArrayDecorator([{foo: 3}, {foo: 4}]) + @methodMultiArrayDecorator({foo: 3}, {foo: 4}) public multiMethod() {} @methodMultiDecorator({a: 'a'}) @@ -556,7 +552,7 @@ describe('MethodMultiDecoratorFactory', () => { public myMethod() {} @methodMultiArrayDecorator({bar: 1}) - @methodMultiArrayDecorator([{bar: 2}, {bar: 3}]) + @methodMultiArrayDecorator({bar: 2}, {bar: 3}) public multiMethod() {} } @@ -608,12 +604,8 @@ describe('MethodMultiDecoratorFactory', () => { }); }); describe('MethodMultiDecoratorFactory for static methods', () => { - function methodMultiArrayDecorator(spec: object | object[]): MethodDecorator { - if (Array.isArray(spec)) { - return MethodMultiDecoratorFactory.createDecorator('test', spec); - } else { - return MethodMultiDecoratorFactory.createDecorator('test', [spec]); - } + function methodMultiArrayDecorator(...spec: object[]): MethodDecorator { + return MethodMultiDecoratorFactory.createDecorator('test', spec); } function methodMultiDecorator(spec: object): MethodDecorator { @@ -626,7 +618,7 @@ describe('MethodMultiDecoratorFactory for static methods', () => { @methodMultiArrayDecorator({foo: 1}) @methodMultiArrayDecorator({foo: 2}) - @methodMultiArrayDecorator([{foo: 3}, {foo: 4}]) + @methodMultiArrayDecorator({foo: 3}, {foo: 4}) static multiMethod() {} @methodMultiDecorator({a: 'a'}) @@ -639,7 +631,7 @@ describe('MethodMultiDecoratorFactory for static methods', () => { static myMethod() {} @methodMultiArrayDecorator({bar: 1}) - @methodMultiArrayDecorator([{bar: 2}, {bar: 3}]) + @methodMultiArrayDecorator({bar: 2}, {bar: 3}) static multiMethod() {} }