From 25704842eb2dc42638feee6f6ffad77340c29f3a Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Wed, 10 Jan 2018 16:47:05 -0800 Subject: [PATCH 01/15] feat(context): enable detection of circular dependencies --- packages/context/src/binding.ts | 37 ++++++- packages/context/src/context.ts | 17 ++- packages/context/src/inject.ts | 10 +- packages/context/src/resolver.ts | 115 +++++++++++++++++--- packages/context/test/unit/resolver.test.ts | 85 +++++++++++++++ 5 files changed, 237 insertions(+), 27 deletions(-) diff --git a/packages/context/src/binding.ts b/packages/context/src/binding.ts index e4ce3d66ec23..59ef38a59237 100644 --- a/packages/context/src/binding.ts +++ b/packages/context/src/binding.ts @@ -4,7 +4,7 @@ // License text available at https://opensource.org/licenses/MIT import {Context} from './context'; -import {Constructor, instantiateClass} from './resolver'; +import {Constructor, instantiateClass, ResolutionSession} from './resolver'; import {isPromise} from './is-promise'; import {Provider} from './provider'; @@ -147,7 +147,10 @@ export class Binding { public type: BindingType; private _cache: BoundValue; - private _getValue: (ctx?: Context) => BoundValue | Promise; + private _getValue: ( + ctx?: Context, + session?: ResolutionSession, + ) => BoundValue | Promise; // For bindings bound via toClass, this property contains the constructor // function @@ -224,8 +227,14 @@ export class Binding { * doSomething(result); * } * ``` + * + * @param ctx Context for the resolution + * @param session Optional session for binding and dependency resolution */ - getValue(ctx: Context): BoundValue | Promise { + getValue( + ctx: Context, + session?: ResolutionSession, + ): BoundValue | Promise { // First check cached value for non-transient if (this._cache !== undefined) { if (this.scope === BindingScope.SINGLETON) { @@ -237,7 +246,22 @@ export class Binding { } } if (this._getValue) { - const result = this._getValue(ctx); + const resolutionSession = ResolutionSession.enterBinding(this, session); + const result = this._getValue(ctx, resolutionSession); + if (isPromise(result)) { + if (result instanceof Promise) { + result.catch(err => { + resolutionSession.exit(); + return Promise.reject(err); + }); + } + result.then(val => { + resolutionSession.exit(); + return val; + }); + } else { + resolutionSession.exit(); + } return this._cacheValue(ctx, result); } return Promise.reject( @@ -347,10 +371,11 @@ export class Binding { */ public toProvider(providerClass: Constructor>): this { this.type = BindingType.PROVIDER; - this._getValue = ctx => { + this._getValue = (ctx, session) => { const providerOrPromise = instantiateClass>( providerClass, ctx!, + session, ); if (isPromise(providerOrPromise)) { return providerOrPromise.then(p => p.value()); @@ -370,7 +395,7 @@ export class Binding { */ toClass(ctor: Constructor): this { this.type = BindingType.CLASS; - this._getValue = ctx => instantiateClass(ctor, ctx!); + this._getValue = (ctx, session) => instantiateClass(ctor, ctx!, session); this.valueConstructor = ctor; return this; } diff --git a/packages/context/src/context.ts b/packages/context/src/context.ts index 3a4f2b6626f0..bb225edd4d2a 100644 --- a/packages/context/src/context.ts +++ b/packages/context/src/context.ts @@ -5,6 +5,7 @@ import {Binding, BoundValue, ValueOrPromise} from './binding'; import {isPromise} from './is-promise'; +import {ResolutionSession} from './resolver'; /** * Context provides an implementation of Inversion of Control (IoC) container @@ -143,9 +144,9 @@ export class Context { * (deeply) nested property to retrieve. * @returns A promise of the bound value. */ - get(key: string): Promise { + get(key: string, session?: ResolutionSession): Promise { try { - return Promise.resolve(this.getValueOrPromise(key)); + return Promise.resolve(this.getValueOrPromise(key, session)); } catch (err) { return Promise.reject(err); } @@ -173,8 +174,8 @@ export class Context { * (deeply) nested property to retrieve. * @returns A promise of the bound value. */ - getSync(key: string): BoundValue { - const valueOrPromise = this.getValueOrPromise(key); + getSync(key: string, session?: ResolutionSession): BoundValue { + const valueOrPromise = this.getValueOrPromise(key, session); if (isPromise(valueOrPromise)) { throw new Error( @@ -227,13 +228,17 @@ export class Context { * * @param keyWithPath The binding key, optionally suffixed with a path to the * (deeply) nested property to retrieve. + * @param session An object to keep states of the resolution * @returns The bound value or a promise of the bound value, depending * on how the binding was configured. * @internal */ - getValueOrPromise(keyWithPath: string): ValueOrPromise { + getValueOrPromise( + keyWithPath: string, + session?: ResolutionSession, + ): ValueOrPromise { const {key, path} = Binding.parseKeyWithPath(keyWithPath); - const boundValue = this.getBinding(key).getValue(this); + const boundValue = this.getBinding(key).getValue(this, session); if (path === undefined || path === '') { return boundValue; } diff --git a/packages/context/src/inject.ts b/packages/context/src/inject.ts index 9ae4a48cd5cc..006eb3a1ce0d 100644 --- a/packages/context/src/inject.ts +++ b/packages/context/src/inject.ts @@ -11,6 +11,8 @@ import { } from '@loopback/metadata'; import {BoundValue, ValueOrPromise} from './binding'; import {Context} from './context'; +import {isPromise} from './is-promise'; +import {ResolutionSession} from './resolver'; const PARAMETERS_KEY = 'inject:parameters'; const PROPERTIES_KEY = 'inject:properties'; @@ -19,7 +21,11 @@ const PROPERTIES_KEY = 'inject:properties'; * A function to provide resolution of injected values */ export interface ResolverFunction { - (ctx: Context, injection: Injection): ValueOrPromise; + ( + ctx: Context, + injection: Injection, + session?: ResolutionSession, + ): ValueOrPromise; } /** @@ -168,12 +174,14 @@ export namespace inject { } function resolveAsGetter(ctx: Context, injection: Injection) { + // No resolution session should be propagated into the getter return function getter() { return ctx.get(injection.bindingKey); }; } function resolveAsSetter(ctx: Context, injection: Injection) { + // No resolution session should be propagated into the setter return function setter(value: BoundValue) { ctx.bind(injection.bindingKey).to(value); }; diff --git a/packages/context/src/resolver.ts b/packages/context/src/resolver.ts index a200971bfa2e..509c0de208f6 100644 --- a/packages/context/src/resolver.ts +++ b/packages/context/src/resolver.ts @@ -4,7 +4,7 @@ // License text available at https://opensource.org/licenses/MIT import {Context} from './context'; -import {BoundValue, ValueOrPromise} from './binding'; +import {BoundValue, ValueOrPromise, Binding} from './binding'; import {isPromise} from './is-promise'; import { describeInjectedArguments, @@ -20,6 +20,68 @@ export type Constructor = // tslint:disable-next-line:no-any new (...args: any[]) => T; +/** + * Object to keep states for a session to resolve bindings and their + * dependencies within a context + */ +export class ResolutionSession { + /** + * A stack of bindings for the current resolution session. It's used to track + * the path of dependency resolution and detect circular dependencies. + */ + readonly bindings: Binding[] = []; + + /** + * Start to resolve a binding within the session + * @param binding Binding + * @param session Resolution session + */ + static enterBinding( + binding: Binding, + session?: ResolutionSession, + ): ResolutionSession { + session = session || new ResolutionSession(); + session.enter(binding); + return session; + } + + /** + * Getter for the current binding + */ + get binding() { + return this.bindings[this.bindings.length - 1]; + } + + /** + * Enter the resolution of the given binding. If + * @param binding Binding + */ + enter(binding: Binding) { + if (this.bindings.indexOf(binding) !== -1) { + throw new Error( + `Circular dependency detected for '${ + binding.key + }' on path '${this.getBindingPath()}'`, + ); + } + this.bindings.push(binding); + } + + /** + * Exit the resolution of a binding + */ + exit() { + return this.bindings.pop(); + } + + /** + * Get the binding path as `bindingA->bindingB->bindingC`. + */ + getBindingPath() { + return this.bindings.map(b => b.key).join('->'); + } +} + /** * Create an instance of a class which constructor has arguments * decorated with `@inject`. @@ -29,16 +91,19 @@ export type Constructor = * * @param ctor The class constructor to call. * @param ctx The context containing values for `@inject` resolution + * @param session Optional session for binding and dependency resolution * @param nonInjectedArgs Optional array of args for non-injected parameters */ export function instantiateClass( ctor: Constructor, ctx: Context, + session?: ResolutionSession, // tslint:disable-next-line:no-any nonInjectedArgs?: any[], ): T | Promise { - const argsOrPromise = resolveInjectedArguments(ctor, ctx, ''); - const propertiesOrPromise = resolveInjectedProperties(ctor, ctx); + session = session || new ResolutionSession(); + const argsOrPromise = resolveInjectedArguments(ctor, '', ctx, session); + const propertiesOrPromise = resolveInjectedProperties(ctor, ctx, session); let inst: T | Promise; if (isPromise(argsOrPromise)) { // Instantiate the class asynchronously @@ -72,14 +137,19 @@ export function instantiateClass( * Resolve the value or promise for a given injection * @param ctx Context * @param injection Descriptor of the injection + * @param session Optional session for binding and dependency resolution */ -function resolve(ctx: Context, injection: Injection): ValueOrPromise { +function resolve( + ctx: Context, + injection: Injection, + session?: ResolutionSession, +): ValueOrPromise { if (injection.resolve) { // A custom resolve function is provided - return injection.resolve(ctx, injection); + return injection.resolve(ctx, injection, session); } // Default to resolve the value from the context by binding key - return ctx.getValueOrPromise(injection.bindingKey); + return ctx.getValueOrPromise(injection.bindingKey, session); } /** @@ -92,16 +162,18 @@ function resolve(ctx: Context, injection: Injection): ValueOrPromise { * * @param target The class for constructor injection or prototype for method * injection - * @param ctx The context containing values for `@inject` resolution * @param method The method name. If set to '', the constructor will * be used. + * @param ctx The context containing values for `@inject` resolution + * @param session Optional session for binding and dependency resolution * @param nonInjectedArgs Optional array of args for non-injected parameters */ export function resolveInjectedArguments( // tslint:disable-next-line:no-any target: any, - ctx: Context, method: string, + ctx: Context, + session?: ResolutionSession, // tslint:disable-next-line:no-any nonInjectedArgs?: any[], ): BoundValue[] | Promise { @@ -137,7 +209,7 @@ export function resolveInjectedArguments( } } - const valueOrPromise = resolve(ctx, injection); + const valueOrPromise = resolve(ctx, injection, session); if (isPromise(valueOrPromise)) { if (!asyncResolvers) asyncResolvers = []; asyncResolvers.push( @@ -173,8 +245,9 @@ export function invokeMethod( ): ValueOrPromise { const argsOrPromise = resolveInjectedArguments( target, - ctx, method, + ctx, + undefined, nonInjectedArgs, ); assert(typeof target[method] === 'function', `Method ${method} not found`); @@ -189,11 +262,24 @@ export function invokeMethod( export type KV = {[p: string]: BoundValue}; +/** + * Given a class with properties decorated with `@inject`, + * return the map of properties resolved using the values + * bound in `ctx`. + + * The function returns an argument array when all dependencies were + * resolved synchronously, or a Promise otherwise. + * + * @param constructor The class for which properties should be resolved. + * @param ctx The context containing values for `@inject` resolution + * @param session Optional session for binding and dependency resolution + */ export function resolveInjectedProperties( - fn: Function, + constructor: Function, ctx: Context, + session?: ResolutionSession, ): KV | Promise { - const injectedProperties = describeInjectedProperties(fn.prototype); + const injectedProperties = describeInjectedProperties(constructor.prototype); const properties: KV = {}; let asyncResolvers: Promise[] | undefined = undefined; @@ -205,11 +291,12 @@ export function resolveInjectedProperties( const injection = injectedProperties[p]; if (!injection.bindingKey && !injection.resolve) { throw new Error( - `Cannot resolve injected property for class ${fn.name}: ` + + `Cannot resolve injected property for class ${constructor.name}: ` + `The property ${p} was not decorated for dependency injection.`, ); } - const valueOrPromise = resolve(ctx, injection); + + const valueOrPromise = resolve(ctx, injection, session); if (isPromise(valueOrPromise)) { if (!asyncResolvers) asyncResolvers = []; asyncResolvers.push(valueOrPromise.then(propertyResolver(p))); diff --git a/packages/context/test/unit/resolver.test.ts b/packages/context/test/unit/resolver.test.ts index bdad238ff2af..7d13cc3a09d1 100644 --- a/packages/context/test/unit/resolver.test.ts +++ b/packages/context/test/unit/resolver.test.ts @@ -88,6 +88,73 @@ describe('constructor injection', () => { const t = instantiateClass(TestClass, ctx) as TestClass; expect(t.fooBar).to.eql('FOO:BAR'); }); + + it('reports circular dependencies of two bindings', () => { + const context = new Context(); + + // Declare two interfaces so that they can be used for typing + interface XInterface {} + interface YInterface {} + + class XClass implements XInterface { + constructor(@inject('y') public y: YInterface) {} + } + + class YClass implements YInterface { + constructor(@inject('x') public x: XInterface) {} + } + + context.bind('x').toClass(XClass); + context.bind('y').toClass(YClass); + expect(() => context.getSync('x')).to.throw(/Circular dependency/); + expect(() => context.getSync('y')).to.throw(/Circular dependency/); + }); + + it('reports circular dependencies of three bindings', () => { + const context = new Context(); + + // Declare interfaces so that they can be used for typing + interface XInterface {} + interface YInterface {} + interface ZInterface {} + + class XClass { + constructor(@inject('y') public y: YInterface) {} + } + + class YClass { + constructor(@inject('z') public z: ZInterface) {} + } + + class ZClass { + constructor(@inject('x') public x: XInterface) {} + } + + context.bind('x').toClass(XClass); + context.bind('y').toClass(YClass); + context.bind('z').toClass(ZClass); + expect(() => context.getSync('x')).to.throw(/Circular dependency/); + expect(() => context.getSync('y')).to.throw(/Circular dependency/); + expect(() => context.getSync('z')).to.throw(/Circular dependency/); + }); + + it('will not report circular dependencies if two bindings', () => { + const context = new Context(); + class XClass {} + + class YClass { + constructor( + @inject('x') public a: XClass, + @inject('x') public b: XClass, + ) {} + } + + context.bind('x').toClass(XClass); + context.bind('y').toClass(YClass); + const y: YClass = context.getSync('y'); + expect(y.a).to.be.instanceof(XClass); + expect(y.b).to.be.instanceof(XClass); + }); }); describe('async constructor injection', () => { @@ -189,6 +256,24 @@ describe('property injection', () => { const t = instantiateClass(TestClass, ctx) as TestClass; expect(t.fooBar).to.eql('FOO:BAR'); }); + + it('reports circular dependencies of two bindings', () => { + const context = new Context(); + class XClass { + // tslint:disable-next-line:no-any + @inject('y') public y: any; + } + + class YClass { + // tslint:disable-next-line:no-any + @inject('x') public x: any; + } + + context.bind('x').toClass(XClass); + context.bind('y').toClass(YClass); + expect(() => context.getSync('x')).to.throw(/Circular dependency/); + expect(() => context.getSync('y')).to.throw(/Circular dependency/); + }); }); describe('async property injection', () => { From 75f1326e124a9534066d571447398c7654ae228c Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Wed, 10 Jan 2018 16:53:56 -0800 Subject: [PATCH 02/15] feat(context): track injections with ResolutionSession --- packages/context/package.json | 4 +- packages/context/src/inject.ts | 73 +++++---- packages/context/src/resolver.ts | 141 ++++++++++++++++-- packages/context/test/unit/resolver.test.ts | 65 ++++++++ packages/metadata/src/decorator-factory.ts | 47 +++--- .../test/unit/decorator-factory.test.ts | 12 +- 6 files changed, 270 insertions(+), 72 deletions(-) diff --git a/packages/context/package.json b/packages/context/package.json index 56dca39df3ce..665af4b8bb0c 100644 --- a/packages/context/package.json +++ b/packages/context/package.json @@ -22,12 +22,14 @@ "author": "IBM", "license": "MIT", "dependencies": { - "@loopback/metadata": "^4.0.0-alpha.4" + "@loopback/metadata": "^4.0.0-alpha.4", + "debug": "^3.1.0" }, "devDependencies": { "@loopback/build": "^4.0.0-alpha.8", "@loopback/testlab": "^4.0.0-alpha.18", "@types/bluebird": "^3.5.18", + "@types/debug": "0.0.30", "bluebird": "^3.5.0" }, "keywords": [ diff --git a/packages/context/src/inject.ts b/packages/context/src/inject.ts index 006eb3a1ce0d..bed5181c0fe0 100644 --- a/packages/context/src/inject.ts +++ b/packages/context/src/inject.ts @@ -5,6 +5,7 @@ import { MetadataInspector, + DecoratorFactory, ParameterDecoratorFactory, PropertyDecoratorFactory, MetadataMap, @@ -32,6 +33,12 @@ export interface ResolverFunction { * Descriptor for an injection point */ export interface Injection { + target: Object; + member?: string | symbol; + methodDescriptorOrParameterIndex?: + | TypedPropertyDescriptor + | number; + bindingKey: string; // Binding key metadata?: {[attribute: string]: BoundValue}; // Related metadata resolve?: ResolverFunction; // A custom resolve function @@ -69,9 +76,8 @@ export function inject( resolve?: ResolverFunction, ) { return function markParameterOrPropertyAsInjected( - // tslint:disable-next-line:no-any - target: any, - propertyKey: string | symbol, + target: Object, + member: string | symbol, methodDescriptorOrParameterIndex?: | TypedPropertyDescriptor | number, @@ -79,38 +85,47 @@ export function inject( if (typeof methodDescriptorOrParameterIndex === 'number') { // The decorator is applied to a method parameter // Please note propertyKey is `undefined` for constructor - const paramDecorator: ParameterDecorator = ParameterDecoratorFactory.createDecorator( - PARAMETERS_KEY, - { - bindingKey, - metadata, - resolve, - }, - ); - paramDecorator(target, propertyKey!, methodDescriptorOrParameterIndex); - } else if (propertyKey) { + const paramDecorator: ParameterDecorator = ParameterDecoratorFactory.createDecorator< + Injection + >(PARAMETERS_KEY, { + target, + member, + methodDescriptorOrParameterIndex, + bindingKey, + metadata, + resolve, + }); + paramDecorator(target, member!, methodDescriptorOrParameterIndex); + } else if (member) { // Property or method - if (typeof Object.getPrototypeOf(target) === 'function') { - const prop = target.name + '.' + propertyKey.toString(); + if (target instanceof Function) { throw new Error( - '@inject is not supported for a static property: ' + prop, + '@inject is not supported for a static property: ' + + DecoratorFactory.getTargetName(target, member), ); } if (methodDescriptorOrParameterIndex) { // Method throw new Error( - '@inject cannot be used on a method: ' + propertyKey.toString(), + '@inject cannot be used on a method: ' + + DecoratorFactory.getTargetName( + target, + member, + methodDescriptorOrParameterIndex, + ), ); } - const propDecorator: PropertyDecorator = PropertyDecoratorFactory.createDecorator( - PROPERTIES_KEY, - { - bindingKey, - metadata, - resolve, - }, - ); - propDecorator(target, propertyKey!); + const propDecorator: PropertyDecorator = PropertyDecoratorFactory.createDecorator< + Injection + >(PROPERTIES_KEY, { + target, + member, + methodDescriptorOrParameterIndex, + bindingKey, + metadata, + resolve, + }); + propDecorator(target, member!); } else { // It won't happen here as `@inject` is not compatible with ClassDecorator /* istanbul ignore next */ @@ -194,8 +209,7 @@ function resolveAsSetter(ctx: Context, injection: Injection) { * @param method Method name, undefined for constructor */ export function describeInjectedArguments( - // tslint:disable-next-line:no-any - target: any, + target: Object, method?: string | symbol, ): Injection[] { method = method || ''; @@ -213,8 +227,7 @@ export function describeInjectedArguments( * prototype for instance properties. */ export function describeInjectedProperties( - // tslint:disable-next-line:no-any - target: any, + target: Object, ): MetadataMap { const metadata = MetadataInspector.getAllPropertyMetadata( diff --git a/packages/context/src/resolver.ts b/packages/context/src/resolver.ts index 509c0de208f6..238d411b7269 100644 --- a/packages/context/src/resolver.ts +++ b/packages/context/src/resolver.ts @@ -12,6 +12,10 @@ import { Injection, } from './inject'; import * as assert from 'assert'; +import * as debugModule from 'debug'; +import {DecoratorFactory} from '@loopback/metadata'; + +const debug = debugModule('loopback:context:resolver'); /** * A class constructor accepting arbitrary arguments. @@ -31,6 +35,8 @@ export class ResolutionSession { */ readonly bindings: Binding[] = []; + readonly injections: Injection[] = []; + /** * Start to resolve a binding within the session * @param binding Binding @@ -45,6 +51,67 @@ export class ResolutionSession { return session; } + /** + * Push an injection into the session + * @param injection Injection + * @param session Resolution session + */ + static enterInjection( + injection: Injection, + session?: ResolutionSession, + ): ResolutionSession { + session = session || new ResolutionSession(); + session.enterInjection(injection); + return session; + } + + private describeInjection(injection?: Injection) { + if (injection == null) return injection; + const name = DecoratorFactory.getTargetName( + injection.target, + injection.member, + injection.methodDescriptorOrParameterIndex, + ); + return { + targetName: name, + bindingKey: injection.bindingKey, + metadata: injection.metadata, + }; + } + + /** + * Push the injection onto the session + * @param injection Injection + */ + enterInjection(injection: Injection) { + if (debug.enabled) { + debug('Enter injection:', this.describeInjection(injection)); + } + this.injections.push(injection); + if (debug.enabled) { + debug('Injection path:', this.getInjectionPath()); + } + } + + /** + * Pop the last injection + */ + exitInjection() { + const injection = this.injections.pop(); + if (debug.enabled) { + debug('Exit injection:', this.describeInjection(injection)); + debug('Injection path:', this.getInjectionPath() || ''); + } + return injection; + } + + /** + * Getter for the current binding + */ + get injection() { + return this.injections[this.injections.length - 1]; + } + /** * Getter for the current binding */ @@ -57,6 +124,9 @@ export class ResolutionSession { * @param binding Binding */ enter(binding: Binding) { + if (debug.enabled) { + debug('Enter binding:', binding.toJSON()); + } if (this.bindings.indexOf(binding) !== -1) { throw new Error( `Circular dependency detected for '${ @@ -65,13 +135,21 @@ export class ResolutionSession { ); } this.bindings.push(binding); + if (debug.enabled) { + debug('Binding path:', this.getBindingPath()); + } } /** * Exit the resolution of a binding */ exit() { - return this.bindings.pop(); + const binding = this.bindings.pop(); + if (debug.enabled) { + debug('Exit binding:', binding && binding.toJSON()); + debug('Binding path:', this.getBindingPath() || ''); + } + return binding; } /** @@ -80,6 +158,15 @@ export class ResolutionSession { getBindingPath() { return this.bindings.map(b => b.key).join('->'); } + + /** + * Get the injection path as `injectionA->injectionB->injectionC`. + */ + getInjectionPath() { + return this.injections + .map(i => this.describeInjection(i)!.targetName) + .join('->'); + } } /** @@ -144,12 +231,30 @@ function resolve( injection: Injection, session?: ResolutionSession, ): ValueOrPromise { + let resolved: ValueOrPromise; + // Push the injection onto the session + session = ResolutionSession.enterInjection(injection, session); if (injection.resolve) { // A custom resolve function is provided - return injection.resolve(ctx, injection, session); + resolved = injection.resolve(ctx, injection, session); + } else { + // Default to resolve the value from the context by binding key + resolved = ctx.getValueOrPromise(injection.bindingKey, session); } - // Default to resolve the value from the context by binding key - return ctx.getValueOrPromise(injection.bindingKey, session); + if (isPromise(resolved)) { + resolved = resolved + .then(r => { + session!.exitInjection(); + return r; + }) + .catch(e => { + session!.exitInjection(); + return Promise.reject(e); + }); + } else { + session.exitInjection(); + } + return resolved; } /** @@ -169,16 +274,19 @@ function resolve( * @param nonInjectedArgs Optional array of args for non-injected parameters */ export function resolveInjectedArguments( - // tslint:disable-next-line:no-any - target: any, + target: Object, method: string, ctx: Context, session?: ResolutionSession, // tslint:disable-next-line:no-any nonInjectedArgs?: any[], ): BoundValue[] | Promise { + const targetWithMethods = <{[method: string]: Function}>target; if (method) { - assert(typeof target[method] === 'function', `Method ${method} not found`); + assert( + typeof targetWithMethods[method] === 'function', + `Method ${method} not found`, + ); } // NOTE: the array may be sparse, i.e. // Object.keys(injectedArgs).length !== injectedArgs.length @@ -187,7 +295,7 @@ export function resolveInjectedArguments( const injectedArgs = describeInjectedArguments(target, method); nonInjectedArgs = nonInjectedArgs || []; - const argLength = method ? target[method].length : target.length; + const argLength = DecoratorFactory.getNumberOfParameters(target, method); const args: BoundValue[] = new Array(argLength); let asyncResolvers: Promise[] | undefined = undefined; @@ -195,14 +303,14 @@ export function resolveInjectedArguments( for (let ix = 0; ix < argLength; ix++) { const injection = ix < injectedArgs.length ? injectedArgs[ix] : undefined; if (injection == null || (!injection.bindingKey && !injection.resolve)) { - const name = method || target.name; if (nonInjectedIndex < nonInjectedArgs.length) { // Set the argument from the non-injected list args[ix] = nonInjectedArgs[nonInjectedIndex++]; continue; } else { + const name = DecoratorFactory.getTargetName(target, method, ix); throw new Error( - `Cannot resolve injected arguments for function ${name}: ` + + `Cannot resolve injected arguments for ${name}: ` + `The arguments[${ix}] is not decorated for dependency injection, ` + `but a value is not supplied`, ); @@ -236,8 +344,7 @@ export function resolveInjectedArguments( * @param nonInjectedArgs Optional array of args for non-injected parameters */ export function invokeMethod( - // tslint:disable-next-line:no-any - target: any, + target: Object, method: string, ctx: Context, // tslint:disable-next-line:no-any @@ -250,13 +357,17 @@ export function invokeMethod( undefined, nonInjectedArgs, ); - assert(typeof target[method] === 'function', `Method ${method} not found`); + const targetWithMethods = <{[method: string]: Function}>target; + assert( + typeof targetWithMethods[method] === 'function', + `Method ${method} not found`, + ); if (isPromise(argsOrPromise)) { // Invoke the target method asynchronously - return argsOrPromise.then(args => target[method](...args)); + return argsOrPromise.then(args => targetWithMethods[method](...args)); } else { // Invoke the target method synchronously - return target[method](...argsOrPromise); + return targetWithMethods[method](...argsOrPromise); } } diff --git a/packages/context/test/unit/resolver.test.ts b/packages/context/test/unit/resolver.test.ts index 7d13cc3a09d1..a93f2213f50f 100644 --- a/packages/context/test/unit/resolver.test.ts +++ b/packages/context/test/unit/resolver.test.ts @@ -11,6 +11,7 @@ import { invokeMethod, Injection, } from '../..'; +import {ResolutionSession} from '../../src/resolver'; describe('constructor injection', () => { let ctx: Context; @@ -155,6 +156,70 @@ describe('constructor injection', () => { expect(y.a).to.be.instanceof(XClass); expect(y.b).to.be.instanceof(XClass); }); + + it('tracks path of bindings', () => { + const context = new Context(); + let bindingPath = ''; + + class ZClass { + @inject( + 'p', + {}, + // Set up a custom resolve() to access information from the session + (c: Context, injection: Injection, session: ResolutionSession) => { + bindingPath = session.getBindingPath(); + }, + ) + myProp: string; + } + + class YClass { + constructor(@inject('z') public z: ZClass) {} + } + + class XClass { + constructor(@inject('y') public y: YClass) {} + } + + context.bind('x').toClass(XClass); + context.bind('y').toClass(YClass); + context.bind('z').toClass(ZClass); + context.getSync('x'); + expect(bindingPath).to.eql('x->y->z'); + }); + + it('tracks path of injections', () => { + const context = new Context(); + let injectionPath = ''; + + class ZClass { + @inject( + 'p', + {}, + // Set up a custom resolve() to access information from the session + (c: Context, injection: Injection, session: ResolutionSession) => { + injectionPath = session.getInjectionPath(); + }, + ) + myProp: string; + } + + class YClass { + constructor(@inject('z') public z: ZClass) {} + } + + class XClass { + constructor(@inject('y') public y: YClass) {} + } + + context.bind('x').toClass(XClass); + context.bind('y').toClass(YClass); + context.bind('z').toClass(ZClass); + context.getSync('x'); + expect(injectionPath).to.eql( + 'XClass.constructor[0]->YClass.constructor[0]->ZClass.prototype.myProp', + ); + }); }); describe('async constructor injection', () => { diff --git a/packages/metadata/src/decorator-factory.ts b/packages/metadata/src/decorator-factory.ts index bbd9a5fd3450..dcfeeac19aff 100644 --- a/packages/metadata/src/decorator-factory.ts +++ b/packages/metadata/src/decorator-factory.ts @@ -123,12 +123,22 @@ export class DecoratorFactory< } /** - * Get name of a decoration target + * Get the qualified name of a decoration target. For example: + * ``` + * class MyClass + * MyClass.constructor[0] // First parameter of the constructor + * MyClass.myStaticProperty + * MyClass.myStaticMethod() + * MyClass.myStaticMethod[0] // First parameter of the myStaticMethod + * MyClass.prototype.myProperty + * MyClass.prototype.myMethod() + * MyClass.prototype.myMethod[1] // Second parameter of myMethod + * ``` * @param target Class or prototype of a class * @param member Optional property/method name * @param descriptorOrIndex Optional method descriptor or parameter index */ - getTargetName( + static getTargetName( target: Object, member?: string | symbol, descriptorOrIndex?: TypedPropertyDescriptor | number, @@ -143,18 +153,11 @@ export class DecoratorFactory< if (member == null) member = 'constructor'; if (typeof descriptorOrIndex === 'number') { // Parameter - name = - 'parameter ' + - name + - '.' + - member.toString() + - '[' + - descriptorOrIndex + - ']'; + name = name + '.' + member.toString() + '[' + descriptorOrIndex + ']'; } else if (descriptorOrIndex != null) { - name = 'method ' + name + '.' + member.toString(); + name = name + '.' + member.toString() + '()'; } else { - name = 'property ' + name + '.' + member.toString(); + name = name + '.' + member.toString(); } return name; } @@ -164,8 +167,8 @@ export class DecoratorFactory< * @param target Class or the prototype * @param member Method name */ - getNumberOfParameters(target: Object, member?: string | symbol) { - if (target instanceof Function && member == null) { + static getNumberOfParameters(target: Object, member?: string | symbol) { + if (target instanceof Function && !member) { // constructor return target.length; } else { @@ -268,7 +271,11 @@ export class DecoratorFactory< member?: string | symbol, descriptorOrIndex?: TypedPropertyDescriptor | number, ) { - const targetName = this.getTargetName(target, member, descriptorOrIndex); + const targetName = DecoratorFactory.getTargetName( + target, + member, + descriptorOrIndex, + ); let meta: M = Reflector.getOwnMetadata(this.key, target); if (meta == null && this.allowInheritance()) { // Clone the base metadata so that it won't be accidentally @@ -349,7 +356,7 @@ export class ClassDecoratorFactory extends DecoratorFactory< if (ownMetadata != null) { throw new Error( 'Decorator cannot be applied more than once on ' + - this.getTargetName(target), + DecoratorFactory.getTargetName(target), ); } return this.withTarget(this.spec, target); @@ -401,7 +408,7 @@ export class PropertyDecoratorFactory extends DecoratorFactory< ) { ownMetadata = ownMetadata || {}; if (ownMetadata[propertyName!] != null) { - const targetName = this.getTargetName(target, propertyName); + const targetName = DecoratorFactory.getTargetName(target, propertyName); throw new Error( 'Decorator cannot be applied more than once on ' + targetName, ); @@ -464,7 +471,7 @@ export class MethodDecoratorFactory extends DecoratorFactory< if (this.getTarget(methodMeta) === target) { throw new Error( 'Decorator cannot be applied more than once on ' + - this.getTargetName(target, methodName, methodDescriptor), + DecoratorFactory.getTargetName(target, methodName, methodDescriptor), ); } // Set the method metadata @@ -513,7 +520,7 @@ export class ParameterDecoratorFactory extends DecoratorFactory< if (methodMeta == null) { // Initialize the method metadata methodMeta = new Array( - this.getNumberOfParameters(target, methodName), + DecoratorFactory.getNumberOfParameters(target, methodName), ).fill(undefined); meta[method] = methodMeta; } @@ -553,7 +560,7 @@ export class ParameterDecoratorFactory extends DecoratorFactory< if (this.getTarget(methodMeta[index]) === target) { throw new Error( 'Decorator cannot be applied more than once on ' + - this.getTargetName(target, methodName, parameterIndex), + DecoratorFactory.getTargetName(target, methodName, parameterIndex), ); } // Set the parameter metadata diff --git a/packages/metadata/test/unit/decorator-factory.test.ts b/packages/metadata/test/unit/decorator-factory.test.ts index e668ede2bca9..67670484f4e7 100644 --- a/packages/metadata/test/unit/decorator-factory.test.ts +++ b/packages/metadata/test/unit/decorator-factory.test.ts @@ -329,7 +329,7 @@ describe('PropertyDecoratorFactory', () => { myProp: string; } }).to.throw( - /Decorator cannot be applied more than once on property MyController\.prototype\.myProp/, + /Decorator cannot be applied more than once on MyController\.prototype\.myProp/, ); }); }); @@ -377,7 +377,7 @@ describe('PropertyDecoratorFactory for static properties', () => { static myProp: string; } }).to.throw( - /Decorator cannot be applied more than once on property MyController\.myProp/, + /Decorator cannot be applied more than once on MyController\.myProp/, ); }); }); @@ -425,7 +425,7 @@ describe('MethodDecoratorFactory', () => { myMethod() {} } }).to.throw( - /Decorator cannot be applied more than once on method MyController\.prototype\.myMethod/, + /Decorator cannot be applied more than once on MyController\.prototype\.myMethod\(\)/, ); }); }); @@ -473,7 +473,7 @@ describe('MethodDecoratorFactory for static methods', () => { static myMethod() {} } }).to.throw( - /Decorator cannot be applied more than once on method MyController\.myMethod/, + /Decorator cannot be applied more than once on MyController\.myMethod\(\)/, ); }); }); @@ -530,7 +530,7 @@ describe('ParameterDecoratorFactory', () => { ) {} } }).to.throw( - /Decorator cannot be applied more than once on parameter MyController\.prototype\.myMethod\[0\]/, + /Decorator cannot be applied more than once on MyController\.prototype\.myMethod\[0\]/, ); }); }); @@ -631,7 +631,7 @@ describe('ParameterDecoratorFactory for a static method', () => { ) {} } }).to.throw( - /Decorator cannot be applied more than once on parameter MyController\.myMethod\[0\]/, + /Decorator cannot be applied more than once on MyController\.myMethod\[0\]/, ); }); }); From cdb0ae4d08bff66d12b93ec3a547d4e759208891 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Wed, 10 Jan 2018 17:03:44 -0800 Subject: [PATCH 03/15] feat(context): add more debug statements --- packages/context/src/binding.ts | 23 ++++ packages/context/src/context.ts | 15 +++ packages/context/src/resolver.ts | 124 ++++++++++++++++---- packages/context/test/unit/resolver.test.ts | 4 +- 4 files changed, 139 insertions(+), 27 deletions(-) diff --git a/packages/context/src/binding.ts b/packages/context/src/binding.ts index 59ef38a59237..a776e54698ed 100644 --- a/packages/context/src/binding.ts +++ b/packages/context/src/binding.ts @@ -8,6 +8,9 @@ import {Constructor, instantiateClass, ResolutionSession} from './resolver'; import {isPromise} from './is-promise'; import {Provider} from './provider'; +import * as debugModule from 'debug'; +const debug = debugModule('loopback:context:binding'); + // tslint:disable-next-line:no-any export type BoundValue = any; @@ -235,6 +238,10 @@ export class Binding { ctx: Context, session?: ResolutionSession, ): BoundValue | Promise { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Get value for binding %s', this.key); + } // First check cached value for non-transient if (this._cache !== undefined) { if (this.scope === BindingScope.SINGLETON) { @@ -324,6 +331,10 @@ export class Binding { 'via ".toDynamicValue()" instead.', ); } + /* istanbul ignore if */ + if (debug.enabled) { + debug('Bind %s to constant:', this.key, value); + } this.type = BindingType.CONSTANT; this._getValue = () => value; return this; @@ -348,6 +359,10 @@ export class Binding { * ``` */ toDynamicValue(factoryFn: () => BoundValue | Promise): this { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Bind %s to dynamic value:', this.key, factoryFn); + } this.type = BindingType.DYNAMIC_VALUE; this._getValue = ctx => factoryFn(); return this; @@ -370,6 +385,10 @@ export class Binding { * @param provider The value provider to use. */ public toProvider(providerClass: Constructor>): this { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Bind %s to provider %s', this.key, providerClass.name); + } this.type = BindingType.PROVIDER; this._getValue = (ctx, session) => { const providerOrPromise = instantiateClass>( @@ -394,6 +413,10 @@ export class Binding { * we can resolve them from the context. */ toClass(ctor: Constructor): this { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Bind %s to class %s', this.key, ctor.name); + } this.type = BindingType.CLASS; this._getValue = (ctx, session) => instantiateClass(ctor, ctx!, session); this.valueConstructor = ctor; diff --git a/packages/context/src/context.ts b/packages/context/src/context.ts index bb225edd4d2a..a6f988ad6f3f 100644 --- a/packages/context/src/context.ts +++ b/packages/context/src/context.ts @@ -7,6 +7,9 @@ import {Binding, BoundValue, ValueOrPromise} from './binding'; import {isPromise} from './is-promise'; import {ResolutionSession} from './resolver'; +import * as debugModule from 'debug'; +const debug = debugModule('loopback:context'); + /** * Context provides an implementation of Inversion of Control (IoC) container */ @@ -28,6 +31,10 @@ export class Context { * @param key Binding key */ bind(key: string): Binding { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Adding binding: %s', key); + } Binding.validateKey(key); const keyExists = this.registry.has(key); if (keyExists) { @@ -145,6 +152,10 @@ export class Context { * @returns A promise of the bound value. */ get(key: string, session?: ResolutionSession): Promise { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Resolving binding: %s', key); + } try { return Promise.resolve(this.getValueOrPromise(key, session)); } catch (err) { @@ -175,6 +186,10 @@ export class Context { * @returns A promise of the bound value. */ getSync(key: string, session?: ResolutionSession): BoundValue { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Resolving binding synchronously: %s', key); + } const valueOrPromise = this.getValueOrPromise(key, session); if (isPromise(valueOrPromise)) { diff --git a/packages/context/src/resolver.ts b/packages/context/src/resolver.ts index 238d411b7269..405d2ebacb8c 100644 --- a/packages/context/src/resolver.ts +++ b/packages/context/src/resolver.ts @@ -16,6 +16,8 @@ import * as debugModule from 'debug'; import {DecoratorFactory} from '@loopback/metadata'; const debug = debugModule('loopback:context:resolver'); +const debugSession = debugModule('loopback:context:resolver:session'); +const getTargetName = DecoratorFactory.getTargetName; /** * A class constructor accepting arbitrary arguments. @@ -65,9 +67,10 @@ export class ResolutionSession { return session; } - private describeInjection(injection?: Injection) { + static describeInjection(injection?: Injection) { + /* istanbul ignore if */ if (injection == null) return injection; - const name = DecoratorFactory.getTargetName( + const name = getTargetName( injection.target, injection.member, injection.methodDescriptorOrParameterIndex, @@ -84,12 +87,17 @@ export class ResolutionSession { * @param injection Injection */ enterInjection(injection: Injection) { - if (debug.enabled) { - debug('Enter injection:', this.describeInjection(injection)); + /* istanbul ignore if */ + if (debugSession.enabled) { + debugSession( + 'Enter injection:', + ResolutionSession.describeInjection(injection), + ); } this.injections.push(injection); - if (debug.enabled) { - debug('Injection path:', this.getInjectionPath()); + /* istanbul ignore if */ + if (debugSession.enabled) { + debugSession('Injection path:', this.getInjectionPath()); } } @@ -98,9 +106,13 @@ export class ResolutionSession { */ exitInjection() { const injection = this.injections.pop(); - if (debug.enabled) { - debug('Exit injection:', this.describeInjection(injection)); - debug('Injection path:', this.getInjectionPath() || ''); + /* istanbul ignore if */ + if (debugSession.enabled) { + debugSession( + 'Exit injection:', + ResolutionSession.describeInjection(injection), + ); + debugSession('Injection path:', this.getInjectionPath() || ''); } return injection; } @@ -124,8 +136,9 @@ export class ResolutionSession { * @param binding Binding */ enter(binding: Binding) { - if (debug.enabled) { - debug('Enter binding:', binding.toJSON()); + /* istanbul ignore if */ + if (debugSession.enabled) { + debugSession('Enter binding:', binding.toJSON()); } if (this.bindings.indexOf(binding) !== -1) { throw new Error( @@ -135,8 +148,9 @@ export class ResolutionSession { ); } this.bindings.push(binding); - if (debug.enabled) { - debug('Binding path:', this.getBindingPath()); + /* istanbul ignore if */ + if (debugSession.enabled) { + debugSession('Binding path:', this.getBindingPath()); } } @@ -145,18 +159,19 @@ export class ResolutionSession { */ exit() { const binding = this.bindings.pop(); - if (debug.enabled) { - debug('Exit binding:', binding && binding.toJSON()); - debug('Binding path:', this.getBindingPath() || ''); + /* istanbul ignore if */ + if (debugSession.enabled) { + debugSession('Exit binding:', binding && binding.toJSON()); + debugSession('Binding path:', this.getBindingPath() || ''); } return binding; } /** - * Get the binding path as `bindingA->bindingB->bindingC`. + * Get the binding path as `bindingA --> bindingB --> bindingC`. */ getBindingPath() { - return this.bindings.map(b => b.key).join('->'); + return this.bindings.map(b => b.key).join(' --> '); } /** @@ -164,8 +179,8 @@ export class ResolutionSession { */ getInjectionPath() { return this.injections - .map(i => this.describeInjection(i)!.targetName) - .join('->'); + .map(i => ResolutionSession.describeInjection(i)!.targetName) + .join(' --> '); } } @@ -188,19 +203,40 @@ export function instantiateClass( // tslint:disable-next-line:no-any nonInjectedArgs?: any[], ): T | Promise { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Instantiating %s', getTargetName(ctor)); + if (nonInjectedArgs && nonInjectedArgs.length) { + debug('Non-injected arguments:', nonInjectedArgs); + } + } session = session || new ResolutionSession(); const argsOrPromise = resolveInjectedArguments(ctor, '', ctx, session); const propertiesOrPromise = resolveInjectedProperties(ctor, ctx, session); let inst: T | Promise; if (isPromise(argsOrPromise)) { // Instantiate the class asynchronously - inst = argsOrPromise.then(args => new ctor(...args)); + inst = argsOrPromise.then(args => { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Injected arguments for %s():', ctor.name, args); + } + return new ctor(...args); + }); } else { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Injected arguments for %s():', ctor.name, argsOrPromise); + } // Instantiate the class synchronously inst = new ctor(...argsOrPromise); } if (isPromise(propertiesOrPromise)) { return propertiesOrPromise.then(props => { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Injected properties for %s:', ctor.name, props); + } if (isPromise(inst)) { // Inject the properties asynchronously return inst.then(obj => Object.assign(obj, props)); @@ -211,6 +247,10 @@ export function instantiateClass( }); } else { if (isPromise(inst)) { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Injected properties for %s:', ctor.name, propertiesOrPromise); + } // Inject the properties asynchronously return inst.then(obj => Object.assign(obj, propertiesOrPromise)); } else { @@ -231,6 +271,13 @@ function resolve( injection: Injection, session?: ResolutionSession, ): ValueOrPromise { + /* istanbul ignore if */ + if (debug.enabled) { + debug( + 'Resolving an injection:', + ResolutionSession.describeInjection(injection), + ); + } let resolved: ValueOrPromise; // Push the injection onto the session session = ResolutionSession.enterInjection(injection, session); @@ -281,6 +328,10 @@ export function resolveInjectedArguments( // tslint:disable-next-line:no-any nonInjectedArgs?: any[], ): BoundValue[] | Promise { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Resolving injected arguments for %s', getTargetName(target, method)); + } const targetWithMethods = <{[method: string]: Function}>target; if (method) { assert( @@ -308,7 +359,7 @@ export function resolveInjectedArguments( args[ix] = nonInjectedArgs[nonInjectedIndex++]; continue; } else { - const name = DecoratorFactory.getTargetName(target, method, ix); + const name = getTargetName(target, method, ix); throw new Error( `Cannot resolve injected arguments for ${name}: ` + `The arguments[${ix}] is not decorated for dependency injection, ` + @@ -350,6 +401,14 @@ export function invokeMethod( // tslint:disable-next-line:no-any nonInjectedArgs?: any[], ): ValueOrPromise { + const methodName = getTargetName(target, method); + /* istanbul ignore if */ + if (debug.enabled) { + debug('Invoking method %s', methodName); + if (nonInjectedArgs && nonInjectedArgs.length) { + debug('Non-injected arguments:', nonInjectedArgs); + } + } const argsOrPromise = resolveInjectedArguments( target, method, @@ -364,8 +423,18 @@ export function invokeMethod( ); if (isPromise(argsOrPromise)) { // Invoke the target method asynchronously - return argsOrPromise.then(args => targetWithMethods[method](...args)); + return argsOrPromise.then(args => { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Injected arguments for %s:', methodName, args); + } + return targetWithMethods[method](...args); + }); } else { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Injected arguments for %s:', methodName, argsOrPromise); + } // Invoke the target method synchronously return targetWithMethods[method](...argsOrPromise); } @@ -390,6 +459,10 @@ export function resolveInjectedProperties( ctx: Context, session?: ResolutionSession, ): KV | Promise { + /* istanbul ignore if */ + if (debug.enabled) { + debug('Resolving injected properties for %s', getTargetName(constructor)); + } const injectedProperties = describeInjectedProperties(constructor.prototype); const properties: KV = {}; @@ -401,9 +474,10 @@ export function resolveInjectedProperties( for (const p in injectedProperties) { const injection = injectedProperties[p]; if (!injection.bindingKey && !injection.resolve) { + const name = getTargetName(constructor, p); throw new Error( - `Cannot resolve injected property for class ${constructor.name}: ` + - `The property ${p} was not decorated for dependency injection.`, + `Cannot resolve injected property ${name}: ` + + `The property ${p} is not decorated for dependency injection.`, ); } diff --git a/packages/context/test/unit/resolver.test.ts b/packages/context/test/unit/resolver.test.ts index a93f2213f50f..771b43d2b647 100644 --- a/packages/context/test/unit/resolver.test.ts +++ b/packages/context/test/unit/resolver.test.ts @@ -185,7 +185,7 @@ describe('constructor injection', () => { context.bind('y').toClass(YClass); context.bind('z').toClass(ZClass); context.getSync('x'); - expect(bindingPath).to.eql('x->y->z'); + expect(bindingPath).to.eql('x --> y --> z'); }); it('tracks path of injections', () => { @@ -217,7 +217,7 @@ describe('constructor injection', () => { context.bind('z').toClass(ZClass); context.getSync('x'); expect(injectionPath).to.eql( - 'XClass.constructor[0]->YClass.constructor[0]->ZClass.prototype.myProp', + 'XClass.constructor[0] --> YClass.constructor[0] --> ZClass.prototype.myProp', ); }); }); From fafb58f3f72b34309c7de0682cbbdc6f1ab5b4ef Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Thu, 21 Dec 2017 11:24:01 -0800 Subject: [PATCH 04/15] fix(metadata): use static methods --- packages/metadata/src/decorator-factory.ts | 11 +++++++++-- packages/metadata/test/unit/decorator-factory.test.ts | 2 +- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/metadata/src/decorator-factory.ts b/packages/metadata/src/decorator-factory.ts index dcfeeac19aff..edd6964df643 100644 --- a/packages/metadata/src/decorator-factory.ts +++ b/packages/metadata/src/decorator-factory.ts @@ -621,7 +621,10 @@ export class MethodParameterDecoratorFactory extends DecoratorFactory< methodName?: string | symbol, methodDescriptor?: TypedPropertyDescriptor | number, ) { - const numOfParams = this.getNumberOfParameters(target, methodName); + const numOfParams = DecoratorFactory.getNumberOfParameters( + target, + methodName, + ); // Fetch the cached parameter index let index = Reflector.getOwnMetadata( this.key + ':index', @@ -632,7 +635,11 @@ export class MethodParameterDecoratorFactory extends DecoratorFactory< if (index == null) index = numOfParams - 1; if (index < 0) { // Excessive decorations than the number of parameters detected - const method = this.getTargetName(target, methodName, methodDescriptor); + const method = DecoratorFactory.getTargetName( + target, + methodName, + methodDescriptor, + ); throw new Error( `The decorator is used more than ${numOfParams} time(s) on ${method}`, ); diff --git a/packages/metadata/test/unit/decorator-factory.test.ts b/packages/metadata/test/unit/decorator-factory.test.ts index 67670484f4e7..6870fac96c1c 100644 --- a/packages/metadata/test/unit/decorator-factory.test.ts +++ b/packages/metadata/test/unit/decorator-factory.test.ts @@ -691,7 +691,7 @@ describe('MethodParameterDecoratorFactory with invalid decorations', () => { myMethod(a: string, b: number) {} } }).to.throw( - /The decorator is used more than 2 time\(s\) on method MyController\.prototype\.myMethod/, + /The decorator is used more than 2 time\(s\) on MyController\.prototype\.myMethod\(\)/, ); }); }); From 9317716f4d9b616d6ad22f6d871bf01e187f042d Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Wed, 10 Jan 2018 17:08:39 -0800 Subject: [PATCH 05/15] fix(context): clean up the circular dependency tests --- packages/context/src/resolver.ts | 4 +- packages/context/test/unit/resolver.test.ts | 46 ++++++++------------- 2 files changed, 19 insertions(+), 31 deletions(-) diff --git a/packages/context/src/resolver.ts b/packages/context/src/resolver.ts index 405d2ebacb8c..9947d300221b 100644 --- a/packages/context/src/resolver.ts +++ b/packages/context/src/resolver.ts @@ -142,9 +142,9 @@ export class ResolutionSession { } if (this.bindings.indexOf(binding) !== -1) { throw new Error( - `Circular dependency detected for '${ + `Circular dependency detected on path '${this.getBindingPath()} --> ${ binding.key - }' on path '${this.getBindingPath()}'`, + }'`, ); } this.bindings.push(binding); diff --git a/packages/context/test/unit/resolver.test.ts b/packages/context/test/unit/resolver.test.ts index 771b43d2b647..b931677a843e 100644 --- a/packages/context/test/unit/resolver.test.ts +++ b/packages/context/test/unit/resolver.test.ts @@ -92,23 +92,25 @@ describe('constructor injection', () => { it('reports circular dependencies of two bindings', () => { const context = new Context(); - - // Declare two interfaces so that they can be used for typing interface XInterface {} interface YInterface {} class XClass implements XInterface { - constructor(@inject('y') public y: YInterface) {} + @inject('y') public y: YInterface; } class YClass implements YInterface { - constructor(@inject('x') public x: XInterface) {} + @inject('x') public x: XInterface; } context.bind('x').toClass(XClass); context.bind('y').toClass(YClass); - expect(() => context.getSync('x')).to.throw(/Circular dependency/); - expect(() => context.getSync('y')).to.throw(/Circular dependency/); + expect(() => context.getSync('x')).to.throw( + "Circular dependency detected on path 'x --> y --> x'", + ); + expect(() => context.getSync('y')).to.throw( + "Circular dependency detected on path 'y --> x --> y'", + ); }); it('reports circular dependencies of three bindings', () => { @@ -134,9 +136,15 @@ describe('constructor injection', () => { context.bind('x').toClass(XClass); context.bind('y').toClass(YClass); context.bind('z').toClass(ZClass); - expect(() => context.getSync('x')).to.throw(/Circular dependency/); - expect(() => context.getSync('y')).to.throw(/Circular dependency/); - expect(() => context.getSync('z')).to.throw(/Circular dependency/); + expect(() => context.getSync('x')).to.throw( + "Circular dependency detected on path 'x --> y --> z --> x'", + ); + expect(() => context.getSync('y')).to.throw( + "Circular dependency detected on path 'y --> z --> x --> y'", + ); + expect(() => context.getSync('z')).to.throw( + "Circular dependency detected on path 'z --> x --> y --> z'", + ); }); it('will not report circular dependencies if two bindings', () => { @@ -321,24 +329,6 @@ describe('property injection', () => { const t = instantiateClass(TestClass, ctx) as TestClass; expect(t.fooBar).to.eql('FOO:BAR'); }); - - it('reports circular dependencies of two bindings', () => { - const context = new Context(); - class XClass { - // tslint:disable-next-line:no-any - @inject('y') public y: any; - } - - class YClass { - // tslint:disable-next-line:no-any - @inject('x') public x: any; - } - - context.bind('x').toClass(XClass); - context.bind('y').toClass(YClass); - expect(() => context.getSync('x')).to.throw(/Circular dependency/); - expect(() => context.getSync('y')).to.throw(/Circular dependency/); - }); }); describe('async property injection', () => { @@ -458,7 +448,6 @@ describe('sync constructor & async property injection', () => { }); function customDecorator(def: Object) { - // tslint:disable-next-line:no-any return inject('foo', def, (c: Context, injection: Injection) => { const barKey = injection.metadata && injection.metadata.x; const b = c.getSync(barKey); @@ -468,7 +457,6 @@ function customDecorator(def: Object) { } function customAsyncDecorator(def: Object) { - // tslint:disable-next-line:no-any return inject('foo', def, async (c: Context, injection: Injection) => { const barKey = injection.metadata && injection.metadata.x; const b = await c.get(barKey); From 534aa818d957f969235e23c8bc078a76dd6cb7ec Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Wed, 10 Jan 2018 17:11:01 -0800 Subject: [PATCH 06/15] fix(context): allow session to be passed into @inject.getter The current session will be cloned to create an isolated scope for the getter function to resume --- packages/context/src/index.ts | 2 +- packages/context/src/inject.ts | 11 +++++-- packages/context/src/resolver.ts | 11 +++++++ packages/context/test/unit/resolver.test.ts | 36 ++++++++++++++++++++- 4 files changed, 55 insertions(+), 5 deletions(-) diff --git a/packages/context/src/index.ts b/packages/context/src/index.ts index 653089723952..147c2b674ce3 100644 --- a/packages/context/src/index.ts +++ b/packages/context/src/index.ts @@ -12,7 +12,7 @@ export { } from './binding'; export {Context} from './context'; -export {Constructor} from './resolver'; +export {Constructor, ResolutionSession} from './resolver'; export {inject, Setter, Getter} from './inject'; export {Provider} from './provider'; export {isPromise} from './is-promise'; diff --git a/packages/context/src/inject.ts b/packages/context/src/inject.ts index bed5181c0fe0..2eba90229949 100644 --- a/packages/context/src/inject.ts +++ b/packages/context/src/inject.ts @@ -188,10 +188,15 @@ export namespace inject { }; } -function resolveAsGetter(ctx: Context, injection: Injection) { - // No resolution session should be propagated into the getter +function resolveAsGetter( + ctx: Context, + injection: Injection, + session?: ResolutionSession, +) { + // We need to clone the session for the getter as it will be resolved later + if (session != null) session = session.clone(); return function getter() { - return ctx.get(injection.bindingKey); + return ctx.get(injection.bindingKey, session); }; } diff --git a/packages/context/src/resolver.ts b/packages/context/src/resolver.ts index 9947d300221b..ef9ae0a6f912 100644 --- a/packages/context/src/resolver.ts +++ b/packages/context/src/resolver.ts @@ -39,6 +39,17 @@ export class ResolutionSession { readonly injections: Injection[] = []; + /** + * Take a snapshot of the ResolutionSession so that we can pass it to + * `@inject.getter` without interferring with the current session + */ + clone() { + const copy = new ResolutionSession(); + copy.bindings.push(...this.bindings); + copy.injections.push(...this.injections); + return copy; + } + /** * Start to resolve a binding within the session * @param binding Binding diff --git a/packages/context/test/unit/resolver.test.ts b/packages/context/test/unit/resolver.test.ts index b931677a843e..88d263852af6 100644 --- a/packages/context/test/unit/resolver.test.ts +++ b/packages/context/test/unit/resolver.test.ts @@ -10,8 +10,10 @@ import { instantiateClass, invokeMethod, Injection, + Constructor, + Getter, + ResolutionSession, } from '../..'; -import {ResolutionSession} from '../../src/resolver'; describe('constructor injection', () => { let ctx: Context; @@ -196,6 +198,38 @@ describe('constructor injection', () => { expect(bindingPath).to.eql('x --> y --> z'); }); + it('tracks path of bindings for @inject.getter', async () => { + const context = new Context(); + let bindingPath = ''; + + class ZClass { + @inject( + 'p', + {}, + // Set up a custom resolve() to access information from the session + (c: Context, injection: Injection, session: ResolutionSession) => { + bindingPath = session.getBindingPath(); + }, + ) + myProp: string; + } + + class YClass { + constructor(@inject.getter('z') public z: Getter) {} + } + + class XClass { + constructor(@inject('y') public y: YClass) {} + } + + context.bind('x').toClass(XClass); + context.bind('y').toClass(YClass); + context.bind('z').toClass(ZClass); + const x: XClass = context.getSync('x'); + await x.y.z(); + expect(bindingPath).to.eql('x --> y --> z'); + }); + it('tracks path of injections', () => { const context = new Context(); let injectionPath = ''; From ee096d501128c8ddeb49f3596a28a7c263498d56 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Wed, 10 Jan 2018 18:02:27 -0800 Subject: [PATCH 07/15] fix: propagate errors via promises --- packages/context/src/binding.ts | 11 +++++++---- packages/context/src/inject.ts | 1 - packages/context/src/resolver.ts | 2 +- packages/context/test/unit/resolver.test.ts | 1 - 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/context/src/binding.ts b/packages/context/src/binding.ts index a776e54698ed..7c42c348f61c 100644 --- a/packages/context/src/binding.ts +++ b/packages/context/src/binding.ts @@ -254,15 +254,18 @@ export class Binding { } if (this._getValue) { const resolutionSession = ResolutionSession.enterBinding(this, session); - const result = this._getValue(ctx, resolutionSession); + let result: ValueOrPromise = this._getValue( + ctx, + resolutionSession, + ); if (isPromise(result)) { if (result instanceof Promise) { - result.catch(err => { + result = result.catch(err => { resolutionSession.exit(); - return Promise.reject(err); + throw err; }); } - result.then(val => { + result = result.then((val: BoundValue) => { resolutionSession.exit(); return val; }); diff --git a/packages/context/src/inject.ts b/packages/context/src/inject.ts index 2eba90229949..0401292400e7 100644 --- a/packages/context/src/inject.ts +++ b/packages/context/src/inject.ts @@ -12,7 +12,6 @@ import { } from '@loopback/metadata'; import {BoundValue, ValueOrPromise} from './binding'; import {Context} from './context'; -import {isPromise} from './is-promise'; import {ResolutionSession} from './resolver'; const PARAMETERS_KEY = 'inject:parameters'; diff --git a/packages/context/src/resolver.ts b/packages/context/src/resolver.ts index ef9ae0a6f912..9729668ac210 100644 --- a/packages/context/src/resolver.ts +++ b/packages/context/src/resolver.ts @@ -307,7 +307,7 @@ function resolve( }) .catch(e => { session!.exitInjection(); - return Promise.reject(e); + throw e; }); } else { session.exitInjection(); diff --git a/packages/context/test/unit/resolver.test.ts b/packages/context/test/unit/resolver.test.ts index 88d263852af6..a90509eddbc3 100644 --- a/packages/context/test/unit/resolver.test.ts +++ b/packages/context/test/unit/resolver.test.ts @@ -10,7 +10,6 @@ import { instantiateClass, invokeMethod, Injection, - Constructor, Getter, ResolutionSession, } from '../..'; From d047b90d195762f77651cfe8fdcedddb88131144 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Thu, 11 Jan 2018 08:56:58 -0800 Subject: [PATCH 08/15] fix: address review comments - Refactor ResolutionSession into its own file - Ensure session.exit is called when an error is thrown - Fix test names --- packages/context/src/binding.ts | 30 +-- packages/context/src/context.ts | 2 +- packages/context/src/index.ts | 3 +- packages/context/src/inject.ts | 2 +- packages/context/src/resolution-session.ts | 180 +++++++++++++++++ packages/context/src/resolver.ts | 204 ++------------------ packages/context/test/unit/resolver.test.ts | 3 +- 7 files changed, 224 insertions(+), 200 deletions(-) create mode 100644 packages/context/src/resolution-session.ts diff --git a/packages/context/src/binding.ts b/packages/context/src/binding.ts index 7c42c348f61c..5cecef18e919 100644 --- a/packages/context/src/binding.ts +++ b/packages/context/src/binding.ts @@ -4,7 +4,8 @@ // License text available at https://opensource.org/licenses/MIT import {Context} from './context'; -import {Constructor, instantiateClass, ResolutionSession} from './resolver'; +import {ResolutionSession} from './resolution-session'; +import {Constructor, instantiateClass} from './resolver'; import {isPromise} from './is-promise'; import {Provider} from './provider'; @@ -254,21 +255,24 @@ export class Binding { } if (this._getValue) { const resolutionSession = ResolutionSession.enterBinding(this, session); - let result: ValueOrPromise = this._getValue( - ctx, - resolutionSession, - ); + let result: ValueOrPromise; + try { + result = this._getValue(ctx, resolutionSession); + } catch (e) { + resolutionSession.exit(); + throw e; + } if (isPromise(result)) { - if (result instanceof Promise) { - result = result.catch(err => { + result = result.then( + (val: BoundValue) => { + resolutionSession.exit(); + return val; + }, + err => { resolutionSession.exit(); throw err; - }); - } - result = result.then((val: BoundValue) => { - resolutionSession.exit(); - return val; - }); + }, + ); } else { resolutionSession.exit(); } diff --git a/packages/context/src/context.ts b/packages/context/src/context.ts index a6f988ad6f3f..0fc75c0dc275 100644 --- a/packages/context/src/context.ts +++ b/packages/context/src/context.ts @@ -5,7 +5,7 @@ import {Binding, BoundValue, ValueOrPromise} from './binding'; import {isPromise} from './is-promise'; -import {ResolutionSession} from './resolver'; +import {ResolutionSession} from './resolution-session'; import * as debugModule from 'debug'; const debug = debugModule('loopback:context'); diff --git a/packages/context/src/index.ts b/packages/context/src/index.ts index 147c2b674ce3..6a0cc6f94461 100644 --- a/packages/context/src/index.ts +++ b/packages/context/src/index.ts @@ -12,7 +12,8 @@ export { } from './binding'; export {Context} from './context'; -export {Constructor, ResolutionSession} from './resolver'; +export {Constructor} from './resolver'; +export {ResolutionSession} from './resolution-session'; export {inject, Setter, Getter} from './inject'; export {Provider} from './provider'; export {isPromise} from './is-promise'; diff --git a/packages/context/src/inject.ts b/packages/context/src/inject.ts index 0401292400e7..9a157f902cde 100644 --- a/packages/context/src/inject.ts +++ b/packages/context/src/inject.ts @@ -12,7 +12,7 @@ import { } from '@loopback/metadata'; import {BoundValue, ValueOrPromise} from './binding'; import {Context} from './context'; -import {ResolutionSession} from './resolver'; +import {ResolutionSession} from './resolution-session'; const PARAMETERS_KEY = 'inject:parameters'; const PROPERTIES_KEY = 'inject:properties'; diff --git a/packages/context/src/resolution-session.ts b/packages/context/src/resolution-session.ts new file mode 100644 index 000000000000..ff31a7b57196 --- /dev/null +++ b/packages/context/src/resolution-session.ts @@ -0,0 +1,180 @@ +// Copyright IBM Corp. 2013, 2018. All Rights Reserved. +// Node module: @loopback/context +// This file is licensed under the MIT License. +// License text available at https://opensource.org/licenses/MIT + +import {Binding} from './binding'; +import {Injection} from './inject'; +import * as debugModule from 'debug'; +import {DecoratorFactory} from '@loopback/metadata'; + +const debugSession = debugModule('loopback:context:resolver:session'); +const getTargetName = DecoratorFactory.getTargetName; +/** + * Object to keep states for a session to resolve bindings and their + * dependencies within a context + */ +export class ResolutionSession { + /** + * A stack of bindings for the current resolution session. It's used to track + * the path of dependency resolution and detect circular dependencies. + */ + readonly bindings: Binding[] = []; + + readonly injections: Injection[] = []; + + /** + * Take a snapshot of the ResolutionSession so that we can pass it to + * `@inject.getter` without interferring with the current session + */ + clone() { + const copy = new ResolutionSession(); + copy.bindings.push(...this.bindings); + copy.injections.push(...this.injections); + return copy; + } + + /** + * Start to resolve a binding within the session + * @param binding Binding + * @param session Resolution session + */ + static enterBinding( + binding: Binding, + session?: ResolutionSession, + ): ResolutionSession { + session = session || new ResolutionSession(); + session.enter(binding); + return session; + } + + /** + * Push an injection into the session + * @param injection Injection + * @param session Resolution session + */ + static enterInjection( + injection: Injection, + session?: ResolutionSession, + ): ResolutionSession { + session = session || new ResolutionSession(); + session.enterInjection(injection); + return session; + } + + static describeInjection(injection?: Injection) { + /* istanbul ignore if */ + if (injection == null) return injection; + const name = getTargetName( + injection.target, + injection.member, + injection.methodDescriptorOrParameterIndex, + ); + return { + targetName: name, + bindingKey: injection.bindingKey, + metadata: injection.metadata, + }; + } + + /** + * Push the injection onto the session + * @param injection Injection + */ + enterInjection(injection: Injection) { + /* istanbul ignore if */ + if (debugSession.enabled) { + debugSession( + 'Enter injection:', + ResolutionSession.describeInjection(injection), + ); + } + this.injections.push(injection); + /* istanbul ignore if */ + if (debugSession.enabled) { + debugSession('Injection path:', this.getInjectionPath()); + } + } + + /** + * Pop the last injection + */ + exitInjection() { + const injection = this.injections.pop(); + /* istanbul ignore if */ + if (debugSession.enabled) { + debugSession( + 'Exit injection:', + ResolutionSession.describeInjection(injection), + ); + debugSession('Injection path:', this.getInjectionPath() || ''); + } + return injection; + } + + /** + * Getter for the current injection + */ + get currentInjection() { + return this.injections[this.injections.length - 1]; + } + + /** + * Getter for the current binding + */ + get currentBinding() { + return this.bindings[this.bindings.length - 1]; + } + + /** + * Enter the resolution of the given binding. If + * @param binding Binding + */ + enter(binding: Binding) { + /* istanbul ignore if */ + if (debugSession.enabled) { + debugSession('Enter binding:', binding.toJSON()); + } + if (this.bindings.indexOf(binding) !== -1) { + throw new Error( + `Circular dependency detected on path '${this.getBindingPath()} --> ${ + binding.key + }'`, + ); + } + this.bindings.push(binding); + /* istanbul ignore if */ + if (debugSession.enabled) { + debugSession('Binding path:', this.getBindingPath()); + } + } + + /** + * Exit the resolution of a binding + */ + exit() { + const binding = this.bindings.pop(); + /* istanbul ignore if */ + if (debugSession.enabled) { + debugSession('Exit binding:', binding && binding.toJSON()); + debugSession('Binding path:', this.getBindingPath() || ''); + } + return binding; + } + + /** + * Get the binding path as `bindingA --> bindingB --> bindingC`. + */ + getBindingPath() { + return this.bindings.map(b => b.key).join(' --> '); + } + + /** + * Get the injection path as `injectionA->injectionB->injectionC`. + */ + getInjectionPath() { + return this.injections + .map(i => ResolutionSession.describeInjection(i)!.targetName) + .join(' --> '); + } +} diff --git a/packages/context/src/resolver.ts b/packages/context/src/resolver.ts index 9729668ac210..f4f682be2474 100644 --- a/packages/context/src/resolver.ts +++ b/packages/context/src/resolver.ts @@ -3,20 +3,21 @@ // This file is licensed under the MIT License. // License text available at https://opensource.org/licenses/MIT +import {DecoratorFactory} from '@loopback/metadata'; import {Context} from './context'; -import {BoundValue, ValueOrPromise, Binding} from './binding'; +import {BoundValue, ValueOrPromise} from './binding'; import {isPromise} from './is-promise'; import { describeInjectedArguments, describeInjectedProperties, Injection, } from './inject'; +import {ResolutionSession} from './resolution-session'; + import * as assert from 'assert'; import * as debugModule from 'debug'; -import {DecoratorFactory} from '@loopback/metadata'; const debug = debugModule('loopback:context:resolver'); -const debugSession = debugModule('loopback:context:resolver:session'); const getTargetName = DecoratorFactory.getTargetName; /** @@ -26,175 +27,6 @@ export type Constructor = // tslint:disable-next-line:no-any new (...args: any[]) => T; -/** - * Object to keep states for a session to resolve bindings and their - * dependencies within a context - */ -export class ResolutionSession { - /** - * A stack of bindings for the current resolution session. It's used to track - * the path of dependency resolution and detect circular dependencies. - */ - readonly bindings: Binding[] = []; - - readonly injections: Injection[] = []; - - /** - * Take a snapshot of the ResolutionSession so that we can pass it to - * `@inject.getter` without interferring with the current session - */ - clone() { - const copy = new ResolutionSession(); - copy.bindings.push(...this.bindings); - copy.injections.push(...this.injections); - return copy; - } - - /** - * Start to resolve a binding within the session - * @param binding Binding - * @param session Resolution session - */ - static enterBinding( - binding: Binding, - session?: ResolutionSession, - ): ResolutionSession { - session = session || new ResolutionSession(); - session.enter(binding); - return session; - } - - /** - * Push an injection into the session - * @param injection Injection - * @param session Resolution session - */ - static enterInjection( - injection: Injection, - session?: ResolutionSession, - ): ResolutionSession { - session = session || new ResolutionSession(); - session.enterInjection(injection); - return session; - } - - static describeInjection(injection?: Injection) { - /* istanbul ignore if */ - if (injection == null) return injection; - const name = getTargetName( - injection.target, - injection.member, - injection.methodDescriptorOrParameterIndex, - ); - return { - targetName: name, - bindingKey: injection.bindingKey, - metadata: injection.metadata, - }; - } - - /** - * Push the injection onto the session - * @param injection Injection - */ - enterInjection(injection: Injection) { - /* istanbul ignore if */ - if (debugSession.enabled) { - debugSession( - 'Enter injection:', - ResolutionSession.describeInjection(injection), - ); - } - this.injections.push(injection); - /* istanbul ignore if */ - if (debugSession.enabled) { - debugSession('Injection path:', this.getInjectionPath()); - } - } - - /** - * Pop the last injection - */ - exitInjection() { - const injection = this.injections.pop(); - /* istanbul ignore if */ - if (debugSession.enabled) { - debugSession( - 'Exit injection:', - ResolutionSession.describeInjection(injection), - ); - debugSession('Injection path:', this.getInjectionPath() || ''); - } - return injection; - } - - /** - * Getter for the current binding - */ - get injection() { - return this.injections[this.injections.length - 1]; - } - - /** - * Getter for the current binding - */ - get binding() { - return this.bindings[this.bindings.length - 1]; - } - - /** - * Enter the resolution of the given binding. If - * @param binding Binding - */ - enter(binding: Binding) { - /* istanbul ignore if */ - if (debugSession.enabled) { - debugSession('Enter binding:', binding.toJSON()); - } - if (this.bindings.indexOf(binding) !== -1) { - throw new Error( - `Circular dependency detected on path '${this.getBindingPath()} --> ${ - binding.key - }'`, - ); - } - this.bindings.push(binding); - /* istanbul ignore if */ - if (debugSession.enabled) { - debugSession('Binding path:', this.getBindingPath()); - } - } - - /** - * Exit the resolution of a binding - */ - exit() { - const binding = this.bindings.pop(); - /* istanbul ignore if */ - if (debugSession.enabled) { - debugSession('Exit binding:', binding && binding.toJSON()); - debugSession('Binding path:', this.getBindingPath() || ''); - } - return binding; - } - - /** - * Get the binding path as `bindingA --> bindingB --> bindingC`. - */ - getBindingPath() { - return this.bindings.map(b => b.key).join(' --> '); - } - - /** - * Get the injection path as `injectionA->injectionB->injectionC`. - */ - getInjectionPath() { - return this.injections - .map(i => ResolutionSession.describeInjection(i)!.targetName) - .join(' --> '); - } -} - /** * Create an instance of a class which constructor has arguments * decorated with `@inject`. @@ -292,23 +124,29 @@ function resolve( let resolved: ValueOrPromise; // Push the injection onto the session session = ResolutionSession.enterInjection(injection, session); - if (injection.resolve) { - // A custom resolve function is provided - resolved = injection.resolve(ctx, injection, session); - } else { - // Default to resolve the value from the context by binding key - resolved = ctx.getValueOrPromise(injection.bindingKey, session); + try { + if (injection.resolve) { + // A custom resolve function is provided + resolved = injection.resolve(ctx, injection, session); + } else { + // Default to resolve the value from the context by binding key + resolved = ctx.getValueOrPromise(injection.bindingKey, session); + } + } catch (e) { + session.exit(); + throw e; } if (isPromise(resolved)) { - resolved = resolved - .then(r => { + resolved = resolved.then( + r => { session!.exitInjection(); return r; - }) - .catch(e => { + }, + e => { session!.exitInjection(); throw e; - }); + }, + ); } else { session.exitInjection(); } diff --git a/packages/context/test/unit/resolver.test.ts b/packages/context/test/unit/resolver.test.ts index a90509eddbc3..fddb73de6ce8 100644 --- a/packages/context/test/unit/resolver.test.ts +++ b/packages/context/test/unit/resolver.test.ts @@ -148,7 +148,8 @@ describe('constructor injection', () => { ); }); - it('will not report circular dependencies if two bindings', () => { + // tslint:disable-next-line:max-line-length + it('will not report circular dependencies if a binding is injected twice', () => { const context = new Context(); class XClass {} From 5f682717bdca7db6aea5ce44006b8c648608814f Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Tue, 9 Jan 2018 10:55:53 -0800 Subject: [PATCH 09/15] test(context): add tests for resolution errors as rejected promises --- packages/context/test/unit/resolver.test.ts | 47 +++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/packages/context/test/unit/resolver.test.ts b/packages/context/test/unit/resolver.test.ts index fddb73de6ce8..25ccf5dc0928 100644 --- a/packages/context/test/unit/resolver.test.ts +++ b/packages/context/test/unit/resolver.test.ts @@ -459,6 +459,53 @@ describe('async constructor & sync property injection', () => { }); }); +describe('async constructor injection with errors', () => { + let ctx: Context; + + before(function() { + ctx = new Context(); + ctx.bind('foo').toDynamicValue( + () => + new Promise((resolve, reject) => { + setImmediate(() => { + reject(new Error('foo: error')); + }); + }), + ); + }); + + it('resolves properties and constructor arguments', async () => { + class TestClass { + constructor(@inject('foo') public foo: string) {} + } + + await expect(instantiateClass(TestClass, ctx)).to.be.rejectedWith( + 'foo: error', + ); + }); +}); + +describe('async property injection with errors', () => { + let ctx: Context; + + before(function() { + ctx = new Context(); + ctx.bind('bar').toDynamicValue(async () => { + throw new Error('bar: error'); + }); + }); + + it('resolves properties and constructor arguments', async () => { + class TestClass { + @inject('bar') bar: string; + } + + await expect(instantiateClass(TestClass, ctx)).to.be.rejectedWith( + 'bar: error', + ); + }); +}); + describe('sync constructor & async property injection', () => { let ctx: Context; From c9d3a2fb11d68f08ba50e51a8c91c1e7ea800dbe Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Thu, 11 Jan 2018 12:23:51 -0800 Subject: [PATCH 10/15] refactor: rename methods per review comments --- packages/context/src/binding.ts | 8 ++++---- packages/context/src/resolution-session.ts | 19 +++++++++++++------ packages/context/src/resolver.ts | 8 ++++---- 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/packages/context/src/binding.ts b/packages/context/src/binding.ts index 5cecef18e919..cf1c9cb63c73 100644 --- a/packages/context/src/binding.ts +++ b/packages/context/src/binding.ts @@ -259,22 +259,22 @@ export class Binding { try { result = this._getValue(ctx, resolutionSession); } catch (e) { - resolutionSession.exit(); + resolutionSession.popBinding(); throw e; } if (isPromise(result)) { result = result.then( (val: BoundValue) => { - resolutionSession.exit(); + resolutionSession.popBinding(); return val; }, err => { - resolutionSession.exit(); + resolutionSession.popBinding(); throw err; }, ); } else { - resolutionSession.exit(); + resolutionSession.popBinding(); } return this._cacheValue(ctx, result); } diff --git a/packages/context/src/resolution-session.ts b/packages/context/src/resolution-session.ts index ff31a7b57196..e5a6777e1336 100644 --- a/packages/context/src/resolution-session.ts +++ b/packages/context/src/resolution-session.ts @@ -21,6 +21,9 @@ export class ResolutionSession { */ readonly bindings: Binding[] = []; + /** + * A stack of injections for the current resolution session. + */ readonly injections: Injection[] = []; /** @@ -44,7 +47,7 @@ export class ResolutionSession { session?: ResolutionSession, ): ResolutionSession { session = session || new ResolutionSession(); - session.enter(binding); + session.pushBinding(binding); return session; } @@ -58,10 +61,14 @@ export class ResolutionSession { session?: ResolutionSession, ): ResolutionSession { session = session || new ResolutionSession(); - session.enterInjection(injection); + session.pushInjection(injection); return session; } + /** + * Describe the injection for debugging purpose + * @param injection + */ static describeInjection(injection?: Injection) { /* istanbul ignore if */ if (injection == null) return injection; @@ -81,7 +88,7 @@ export class ResolutionSession { * Push the injection onto the session * @param injection Injection */ - enterInjection(injection: Injection) { + pushInjection(injection: Injection) { /* istanbul ignore if */ if (debugSession.enabled) { debugSession( @@ -99,7 +106,7 @@ export class ResolutionSession { /** * Pop the last injection */ - exitInjection() { + popInjection() { const injection = this.injections.pop(); /* istanbul ignore if */ if (debugSession.enabled) { @@ -130,7 +137,7 @@ export class ResolutionSession { * Enter the resolution of the given binding. If * @param binding Binding */ - enter(binding: Binding) { + pushBinding(binding: Binding) { /* istanbul ignore if */ if (debugSession.enabled) { debugSession('Enter binding:', binding.toJSON()); @@ -152,7 +159,7 @@ export class ResolutionSession { /** * Exit the resolution of a binding */ - exit() { + popBinding() { const binding = this.bindings.pop(); /* istanbul ignore if */ if (debugSession.enabled) { diff --git a/packages/context/src/resolver.ts b/packages/context/src/resolver.ts index f4f682be2474..962df8bdf437 100644 --- a/packages/context/src/resolver.ts +++ b/packages/context/src/resolver.ts @@ -133,22 +133,22 @@ function resolve( resolved = ctx.getValueOrPromise(injection.bindingKey, session); } } catch (e) { - session.exit(); + session.popBinding(); throw e; } if (isPromise(resolved)) { resolved = resolved.then( r => { - session!.exitInjection(); + session!.popInjection(); return r; }, e => { - session!.exitInjection(); + session!.popInjection(); throw e; }, ); } else { - session.exitInjection(); + session.popInjection(); } return resolved; } From 39debc579a758826a626c430682debd777d2c75d Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Thu, 11 Jan 2018 15:08:56 -0800 Subject: [PATCH 11/15] refactor: add helper methods to do work with binding/injection --- packages/context/src/binding.ts | 27 ++----- packages/context/src/resolution-session.ts | 91 +++++++++++++++++++++- packages/context/src/resolver.ts | 42 ++++------ 3 files changed, 106 insertions(+), 54 deletions(-) diff --git a/packages/context/src/binding.ts b/packages/context/src/binding.ts index cf1c9cb63c73..eef6e1c1f23d 100644 --- a/packages/context/src/binding.ts +++ b/packages/context/src/binding.ts @@ -254,28 +254,11 @@ export class Binding { } } if (this._getValue) { - const resolutionSession = ResolutionSession.enterBinding(this, session); - let result: ValueOrPromise; - try { - result = this._getValue(ctx, resolutionSession); - } catch (e) { - resolutionSession.popBinding(); - throw e; - } - if (isPromise(result)) { - result = result.then( - (val: BoundValue) => { - resolutionSession.popBinding(); - return val; - }, - err => { - resolutionSession.popBinding(); - throw err; - }, - ); - } else { - resolutionSession.popBinding(); - } + let result = ResolutionSession.runWithBinding( + s => this._getValue(ctx, s), + this, + session, + ); return this._cacheValue(ctx, result); } return Promise.reject( diff --git a/packages/context/src/resolution-session.ts b/packages/context/src/resolution-session.ts index e5a6777e1336..f1544c11fb80 100644 --- a/packages/context/src/resolution-session.ts +++ b/packages/context/src/resolution-session.ts @@ -3,13 +3,59 @@ // This file is licensed under the MIT License. // License text available at https://opensource.org/licenses/MIT -import {Binding} from './binding'; +import {Binding, ValueOrPromise, BoundValue} from './binding'; import {Injection} from './inject'; +import {isPromise} from './is-promise'; import * as debugModule from 'debug'; import {DecoratorFactory} from '@loopback/metadata'; const debugSession = debugModule('loopback:context:resolver:session'); const getTargetName = DecoratorFactory.getTargetName; + +/** + * A function to be executed with the resolution session + */ +export type ResolutionAction = ( + session?: ResolutionSession, +) => ValueOrPromise; + +/** + * Try to run an action that returns a promise or a value + * @param action A function that returns a promise or a value + * @param finalAction A function to be called once the action + * is fulfilled or rejected (synchronously or asynchronously) + */ +function tryWithFinally( + action: () => ValueOrPromise, + finalAction: () => void, +) { + let result: ValueOrPromise; + try { + result = action(); + } catch (err) { + finalAction(); + throw err; + } + if (isPromise(result)) { + // Once (promise.finally)[https://github.com/tc39/proposal-promise-finally + // is supported, the following can be simplifed as + // `result = result.finally(finalAction);` + result = result.then( + val => { + finalAction(); + return val; + }, + err => { + finalAction(); + throw err; + }, + ); + } else { + finalAction(); + } + return result; +} + /** * Object to keep states for a session to resolve bindings and their * dependencies within a context @@ -42,7 +88,7 @@ export class ResolutionSession { * @param binding Binding * @param session Resolution session */ - static enterBinding( + private static enterBinding( binding: Binding, session?: ResolutionSession, ): ResolutionSession { @@ -51,12 +97,30 @@ export class ResolutionSession { return session; } + /** + * Run the given action with the given binding and session + * @param action A function to do some work with the resolution session + * @param binding The current binding + * @param session The current resolution session + */ + static runWithBinding( + action: ResolutionAction, + binding: Binding, + session?: ResolutionSession, + ) { + const resolutionSession = ResolutionSession.enterBinding(binding, session); + return tryWithFinally( + () => action(resolutionSession), + () => resolutionSession.popBinding(), + ); + } + /** * Push an injection into the session * @param injection Injection * @param session Resolution session */ - static enterInjection( + private static enterInjection( injection: Injection, session?: ResolutionSession, ): ResolutionSession { @@ -65,6 +129,27 @@ export class ResolutionSession { return session; } + /** + * Run the given action with the given injection and session + * @param action A function to do some work with the resolution session + * @param binding The current injection + * @param session The current resolution session + */ + static runWithInjection( + action: ResolutionAction, + injection: Injection, + session?: ResolutionSession, + ) { + const resolutionSession = ResolutionSession.enterInjection( + injection, + session, + ); + return tryWithFinally( + () => action(resolutionSession), + () => resolutionSession.popInjection(), + ); + } + /** * Describe the injection for debugging purpose * @param injection diff --git a/packages/context/src/resolver.ts b/packages/context/src/resolver.ts index 962df8bdf437..ca13bff007ca 100644 --- a/packages/context/src/resolver.ts +++ b/packages/context/src/resolver.ts @@ -121,35 +121,19 @@ function resolve( ResolutionSession.describeInjection(injection), ); } - let resolved: ValueOrPromise; - // Push the injection onto the session - session = ResolutionSession.enterInjection(injection, session); - try { - if (injection.resolve) { - // A custom resolve function is provided - resolved = injection.resolve(ctx, injection, session); - } else { - // Default to resolve the value from the context by binding key - resolved = ctx.getValueOrPromise(injection.bindingKey, session); - } - } catch (e) { - session.popBinding(); - throw e; - } - if (isPromise(resolved)) { - resolved = resolved.then( - r => { - session!.popInjection(); - return r; - }, - e => { - session!.popInjection(); - throw e; - }, - ); - } else { - session.popInjection(); - } + let resolved = ResolutionSession.runWithInjection( + s => { + if (injection.resolve) { + // A custom resolve function is provided + return injection.resolve(ctx, injection, s); + } else { + // Default to resolve the value from the context by binding key + return ctx.getValueOrPromise(injection.bindingKey, s); + } + }, + injection, + session, + ); return resolved; } From f577941c2ab77856d5038b9b012dfc6e96802006 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Fri, 12 Jan 2018 08:32:13 -0800 Subject: [PATCH 12/15] fix: simplify string formatting --- packages/metadata/src/decorator-factory.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/metadata/src/decorator-factory.ts b/packages/metadata/src/decorator-factory.ts index edd6964df643..ebb08a9ec84a 100644 --- a/packages/metadata/src/decorator-factory.ts +++ b/packages/metadata/src/decorator-factory.ts @@ -146,18 +146,18 @@ export class DecoratorFactory< let name = target instanceof Function ? target.name - : target.constructor.name + '.prototype'; + : `${target.constructor.name}.prototype`; if (member == null && descriptorOrIndex == null) { - return 'class ' + name; + return `class ${name}`; } if (member == null) member = 'constructor'; if (typeof descriptorOrIndex === 'number') { // Parameter - name = name + '.' + member.toString() + '[' + descriptorOrIndex + ']'; + name = `${name}.${member}[${descriptorOrIndex}]`; } else if (descriptorOrIndex != null) { - name = name + '.' + member.toString() + '()'; + name = `${name}.${member}()`; } else { - name = name + '.' + member.toString(); + name = `${name}.${member}`; } return name; } @@ -627,7 +627,7 @@ export class MethodParameterDecoratorFactory extends DecoratorFactory< ); // Fetch the cached parameter index let index = Reflector.getOwnMetadata( - this.key + ':index', + `${this.key}:index`, target, methodName, ); @@ -667,7 +667,7 @@ export class MethodParameterDecoratorFactory extends DecoratorFactory< } // Cache the index to help us position the next parameter Reflector.defineMetadata( - this.key + ':index', + `${this.key}:index`, index - 1, target, methodName, @@ -691,7 +691,7 @@ export class MethodParameterDecoratorFactory extends DecoratorFactory< ownMetadata[methodName!] = params; // Cache the index to help us position the next parameter Reflector.defineMetadata( - this.key + ':index', + `${this.key}:index`, index - 1, target, methodName, From 948da90b9aa0540c32fe27b22a89b5e658792e70 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Fri, 12 Jan 2018 10:05:48 -0800 Subject: [PATCH 13/15] feat(context): use one stack to track bindings and injections --- packages/context/src/inject.ts | 2 +- packages/context/src/resolution-session.ts | 134 +++++++++++++++----- packages/context/src/resolver.ts | 15 ++- packages/context/test/unit/resolver.test.ts | 12 ++ 4 files changed, 125 insertions(+), 38 deletions(-) diff --git a/packages/context/src/inject.ts b/packages/context/src/inject.ts index 9a157f902cde..16214e300c9e 100644 --- a/packages/context/src/inject.ts +++ b/packages/context/src/inject.ts @@ -193,7 +193,7 @@ function resolveAsGetter( session?: ResolutionSession, ) { // We need to clone the session for the getter as it will be resolved later - if (session != null) session = session.clone(); + session = ResolutionSession.fork(session); return function getter() { return ctx.get(injection.bindingKey, session); }; diff --git a/packages/context/src/resolution-session.ts b/packages/context/src/resolution-session.ts index f1544c11fb80..9c5f4ae8c448 100644 --- a/packages/context/src/resolution-session.ts +++ b/packages/context/src/resolution-session.ts @@ -56,6 +56,27 @@ function tryWithFinally( return result; } +/** + * Wrapper for bindings tracked by resolution sessions + */ +export interface BindingElement { + type: 'binding'; + value: Binding; +} + +/** + * Wrapper for injections tracked by resolution sessions + */ +export interface InjectionElement { + type: 'injection'; + value: Injection; +} + +/** + * Binding or injection elements tracked by resolution sessions + */ +export type ResolutionElement = BindingElement | InjectionElement; + /** * Object to keep states for a session to resolve bindings and their * dependencies within a context @@ -65,28 +86,25 @@ export class ResolutionSession { * A stack of bindings for the current resolution session. It's used to track * the path of dependency resolution and detect circular dependencies. */ - readonly bindings: Binding[] = []; - - /** - * A stack of injections for the current resolution session. - */ - readonly injections: Injection[] = []; + readonly stack: ResolutionElement[] = []; /** - * Take a snapshot of the ResolutionSession so that we can pass it to - * `@inject.getter` without interferring with the current session + * Fork the current session so that a new one with the same stack can be used + * in parallel or future resolutions, such as multiple method arguments, + * multiple properties, or a getter function + * @param session The current session */ - clone() { + static fork(session?: ResolutionSession): ResolutionSession | undefined { + if (session === undefined) return undefined; const copy = new ResolutionSession(); - copy.bindings.push(...this.bindings); - copy.injections.push(...this.injections); + copy.stack.push(...session.stack); return copy; } /** * Start to resolve a binding within the session - * @param binding Binding - * @param session Resolution session + * @param binding The current binding + * @param session The current resolution session */ private static enterBinding( binding: Binding, @@ -117,8 +135,8 @@ export class ResolutionSession { /** * Push an injection into the session - * @param injection Injection - * @param session Resolution session + * @param injection The current injection + * @param session The current resolution session */ private static enterInjection( injection: Injection, @@ -152,7 +170,7 @@ export class ResolutionSession { /** * Describe the injection for debugging purpose - * @param injection + * @param injection Injection object */ static describeInjection(injection?: Injection) { /* istanbul ignore if */ @@ -171,7 +189,7 @@ export class ResolutionSession { /** * Push the injection onto the session - * @param injection Injection + * @param injection Injection The current injection */ pushInjection(injection: Injection) { /* istanbul ignore if */ @@ -181,10 +199,10 @@ export class ResolutionSession { ResolutionSession.describeInjection(injection), ); } - this.injections.push(injection); + this.stack.push({type: 'injection', value: injection}); /* istanbul ignore if */ if (debugSession.enabled) { - debugSession('Injection path:', this.getInjectionPath()); + debugSession('Resolution path:', this.getResolutionPath()); } } @@ -192,14 +210,19 @@ export class ResolutionSession { * Pop the last injection */ popInjection() { - const injection = this.injections.pop(); + const top = this.stack.pop(); + if (top === undefined || top.type !== 'injection') { + throw new Error('The top element must be an injection'); + } + + const injection = top.value; /* istanbul ignore if */ if (debugSession.enabled) { debugSession( 'Exit injection:', ResolutionSession.describeInjection(injection), ); - debugSession('Injection path:', this.getInjectionPath() || ''); + debugSession('Resolution path:', this.getResolutionPath() || ''); } return injection; } @@ -207,15 +230,29 @@ export class ResolutionSession { /** * Getter for the current injection */ - get currentInjection() { - return this.injections[this.injections.length - 1]; + get currentInjection(): Injection | undefined { + for (let i = this.stack.length - 1; i >= 0; i--) { + const element = this.stack[i]; + switch (element.type) { + case 'injection': + return element.value; + } + } + return undefined; } /** * Getter for the current binding */ - get currentBinding() { - return this.bindings[this.bindings.length - 1]; + get currentBinding(): Binding | undefined { + for (let i = this.stack.length - 1; i >= 0; i--) { + const element = this.stack[i]; + switch (element.type) { + case 'binding': + return element.value; + } + } + return undefined; } /** @@ -227,17 +264,17 @@ export class ResolutionSession { if (debugSession.enabled) { debugSession('Enter binding:', binding.toJSON()); } - if (this.bindings.indexOf(binding) !== -1) { + if (this.stack.find(i => i.type === 'binding' && i.value === binding)) { throw new Error( `Circular dependency detected on path '${this.getBindingPath()} --> ${ binding.key }'`, ); } - this.bindings.push(binding); + this.stack.push({type: 'binding', value: binding}); /* istanbul ignore if */ if (debugSession.enabled) { - debugSession('Binding path:', this.getBindingPath()); + debugSession('Resolution path:', this.getResolutionPath()); } } @@ -245,11 +282,15 @@ export class ResolutionSession { * Exit the resolution of a binding */ popBinding() { - const binding = this.bindings.pop(); + const top = this.stack.pop(); + if (top === undefined || top.type !== 'binding') { + throw new Error('The top element must be a binding'); + } + const binding = top.value; /* istanbul ignore if */ if (debugSession.enabled) { debugSession('Exit binding:', binding && binding.toJSON()); - debugSession('Binding path:', this.getBindingPath() || ''); + debugSession('Resolution path:', this.getResolutionPath() || ''); } return binding; } @@ -258,15 +299,40 @@ export class ResolutionSession { * Get the binding path as `bindingA --> bindingB --> bindingC`. */ getBindingPath() { - return this.bindings.map(b => b.key).join(' --> '); + return this.stack + .filter(i => i.type === 'binding') + .map(b => (b.value).key) + .join(' --> '); } /** - * Get the injection path as `injectionA->injectionB->injectionC`. + * Get the injection path as `injectionA --> injectionB --> injectionC`. */ getInjectionPath() { - return this.injections - .map(i => ResolutionSession.describeInjection(i)!.targetName) + return this.stack + .filter(i => i.type === 'injection') + .map( + i => + ResolutionSession.describeInjection(i.value)!.targetName, + ) .join(' --> '); } + + private static describe(e: ResolutionElement) { + switch (e.type) { + case 'injection': + return '@' + ResolutionSession.describeInjection(e.value)!.targetName; + case 'binding': + return e.value.key; + } + } + + /** + * Get the resolution path including bindings and injections, for example: + * `bindingA --> @ClassA[0] --> bindingB --> @ClassB.prototype.prop1 + * --> bindingC`. + */ + getResolutionPath() { + return this.stack.map(i => ResolutionSession.describe(i)).join(' --> '); + } } diff --git a/packages/context/src/resolver.ts b/packages/context/src/resolver.ts index ca13bff007ca..710c5c6990cf 100644 --- a/packages/context/src/resolver.ts +++ b/packages/context/src/resolver.ts @@ -53,7 +53,6 @@ export function instantiateClass( debug('Non-injected arguments:', nonInjectedArgs); } } - session = session || new ResolutionSession(); const argsOrPromise = resolveInjectedArguments(ctor, '', ctx, session); const propertiesOrPromise = resolveInjectedProperties(ctor, ctx, session); let inst: T | Promise; @@ -201,7 +200,12 @@ export function resolveInjectedArguments( } } - const valueOrPromise = resolve(ctx, injection, session); + // Clone the session so that multiple arguments can be resolved in parallel + const valueOrPromise = resolve( + ctx, + injection, + ResolutionSession.fork(session), + ); if (isPromise(valueOrPromise)) { if (!asyncResolvers) asyncResolvers = []; asyncResolvers.push( @@ -314,7 +318,12 @@ export function resolveInjectedProperties( ); } - const valueOrPromise = resolve(ctx, injection, session); + // Clone the session so that multiple properties can be resolved in parallel + const valueOrPromise = resolve( + ctx, + injection, + ResolutionSession.fork(session), + ); if (isPromise(valueOrPromise)) { if (!asyncResolvers) asyncResolvers = []; asyncResolvers.push(valueOrPromise.then(propertyResolver(p))); diff --git a/packages/context/test/unit/resolver.test.ts b/packages/context/test/unit/resolver.test.ts index 25ccf5dc0928..0e446796192c 100644 --- a/packages/context/test/unit/resolver.test.ts +++ b/packages/context/test/unit/resolver.test.ts @@ -170,6 +170,7 @@ describe('constructor injection', () => { it('tracks path of bindings', () => { const context = new Context(); let bindingPath = ''; + let resolutionPath = ''; class ZClass { @inject( @@ -178,6 +179,7 @@ describe('constructor injection', () => { // Set up a custom resolve() to access information from the session (c: Context, injection: Injection, session: ResolutionSession) => { bindingPath = session.getBindingPath(); + resolutionPath = session.getResolutionPath(); }, ) myProp: string; @@ -196,11 +198,16 @@ describe('constructor injection', () => { context.bind('z').toClass(ZClass); context.getSync('x'); expect(bindingPath).to.eql('x --> y --> z'); + expect(resolutionPath).to.eql( + 'x --> @XClass.constructor[0] --> y --> @YClass.constructor[0]' + + ' --> z --> @ZClass.prototype.myProp', + ); }); it('tracks path of bindings for @inject.getter', async () => { const context = new Context(); let bindingPath = ''; + let resolutionPath = ''; class ZClass { @inject( @@ -209,6 +216,7 @@ describe('constructor injection', () => { // Set up a custom resolve() to access information from the session (c: Context, injection: Injection, session: ResolutionSession) => { bindingPath = session.getBindingPath(); + resolutionPath = session.getResolutionPath(); }, ) myProp: string; @@ -228,6 +236,10 @@ describe('constructor injection', () => { const x: XClass = context.getSync('x'); await x.y.z(); expect(bindingPath).to.eql('x --> y --> z'); + expect(resolutionPath).to.eql( + 'x --> @XClass.constructor[0] --> y --> @YClass.constructor[0]' + + ' --> z --> @ZClass.prototype.myProp', + ); }); it('tracks path of injections', () => { From 156a4b6ab62f2b09ac6e0565b17bdfb698419a17 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Fri, 12 Jan 2018 10:14:29 -0800 Subject: [PATCH 14/15] fix(context): fix the test to avoid UnhandledPromiseRejectionWarning --- packages/context/test/unit/binding.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/context/test/unit/binding.ts b/packages/context/test/unit/binding.ts index ef7786ab09a0..63d0d60860c1 100644 --- a/packages/context/test/unit/binding.ts +++ b/packages/context/test/unit/binding.ts @@ -111,9 +111,14 @@ describe('Binding', () => { }); it('rejects rejected promise values', () => { - expect(() => binding.to(Promise.reject('error'))).to.throw( + const p = Promise.reject('error'); + expect(() => binding.to(p)).to.throw( /Promise instances are not allowed.*toDynamicValue/, ); + // Catch the rejected promise to avoid + // (node:60994) UnhandledPromiseRejectionWarning: Unhandled promise + // rejection (rejection id: 1): error + p.catch(e => {}); }); }); From f4857c5c69d4f4261cf16f764ce4f93f11d217fb Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Mon, 15 Jan 2018 11:11:43 -0800 Subject: [PATCH 15/15] fix(context): add resolution-session.ts for api docs --- packages/context/docs.json | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/context/docs.json b/packages/context/docs.json index 857bea3c1f0b..a6a744fca073 100644 --- a/packages/context/docs.json +++ b/packages/context/docs.json @@ -8,6 +8,7 @@ "src/is-promise.ts", "src/provider.ts", "src/reflect.ts", + "src/resolution-session.ts", "src/resolver.ts" ], "codeSectionDepth": 4,