From 04baa46efe5614ee037fffbf3a02afdd4fde0e55 Mon Sep 17 00:00:00 2001 From: vsavkin Date: Mon, 13 Jul 2015 15:48:28 -0700 Subject: [PATCH] fix(di): removed default visibility BREAKING CHANGE: Directives will use the Unbounded visibility by default, whereas before the change they used Self --- modules/angular2/di.ts | 3 +-- .../src/core/annotations_impl/annotations.ts | 4 +-- .../src/core/compiler/element_injector.ts | 3 +-- modules/angular2/src/di/binding.ts | 24 +++-------------- modules/angular2/src/di/decorators.dart | 2 +- modules/angular2/src/di/decorators.ts | 4 +-- modules/angular2/src/di/injector.ts | 8 +++--- modules/angular2/src/di/metadata.ts | 6 ++--- .../core/compiler/element_injector_spec.ts | 14 +++++----- modules/angular2/test/di/injector_spec.ts | 27 +------------------ 10 files changed, 26 insertions(+), 69 deletions(-) diff --git a/modules/angular2/di.ts b/modules/angular2/di.ts index da85b8ddf3..cc8220464e 100644 --- a/modules/angular2/di.ts +++ b/modules/angular2/di.ts @@ -14,8 +14,7 @@ export { AncestorMetadata, UnboundedMetadata, DependencyMetadata, - self, - unbounded + DEFAULT_VISIBILITY } from './src/di/metadata'; // we have to reexport * because Dart and TS export two different sets of types diff --git a/modules/angular2/src/core/annotations_impl/annotations.ts b/modules/angular2/src/core/annotations_impl/annotations.ts index 26dc28a0cd..3408a7e555 100644 --- a/modules/angular2/src/core/annotations_impl/annotations.ts +++ b/modules/angular2/src/core/annotations_impl/annotations.ts @@ -1,6 +1,6 @@ import {CONST, CONST_EXPR} from 'angular2/src/facade/lang'; import {List} from 'angular2/src/facade/collection'; -import {InjectableMetadata, self} from 'angular2/src/di/metadata'; +import {InjectableMetadata} from 'angular2/src/di/metadata'; import {DEFAULT} from 'angular2/change_detection'; /** @@ -792,7 +792,7 @@ export class Directive extends InjectableMetadata { exportAs?: string, compileChildren?: boolean, } = {}) { - super(self); + super(); this.selector = selector; this.properties = properties; this.events = events; diff --git a/modules/angular2/src/core/compiler/element_injector.ts b/modules/angular2/src/core/compiler/element_injector.ts index 53638b0e4b..59b092d57c 100644 --- a/modules/angular2/src/core/compiler/element_injector.ts +++ b/modules/angular2/src/core/compiler/element_injector.ts @@ -26,8 +26,7 @@ import { CyclicDependencyError, resolveForwardRef, VisibilityMetadata, - DependencyProvider, - self + DependencyProvider } from 'angular2/di'; import { InjectorInlineStrategy, diff --git a/modules/angular2/src/di/binding.ts b/modules/angular2/src/di/binding.ts index 5751e8d8d5..3e5a00ce18 100644 --- a/modules/angular2/src/di/binding.ts +++ b/modules/angular2/src/di/binding.ts @@ -16,7 +16,7 @@ import { InjectableMetadata, VisibilityMetadata, OptionalMetadata, - unbounded, + DEFAULT_VISIBILITY, DependencyMetadata } from './metadata'; import {NoAnnotationError} from './exceptions'; @@ -30,7 +30,7 @@ export class Dependency { public properties: List) {} static fromKey(key: Key): Dependency { - return new Dependency(key, false, _defaulVisiblity(key.token), []); + return new Dependency(key, false, DEFAULT_VISIBILITY, []); } } @@ -397,18 +397,16 @@ function _extractToken(typeOrFunc, annotations /*List | any*/, params: List var optional = false; if (!isArray(annotations)) { - return _createDependency(annotations, optional, _defaulVisiblity(annotations), depProps); + return _createDependency(annotations, optional, DEFAULT_VISIBILITY, depProps); } - var visibility = null; - var defaultVisibility = unbounded; + var visibility = DEFAULT_VISIBILITY; for (var i = 0; i < annotations.length; ++i) { var paramAnnotation = annotations[i]; if (paramAnnotation instanceof Type) { token = paramAnnotation; - defaultVisibility = _defaulVisiblity(token); } else if (paramAnnotation instanceof InjectMetadata) { token = paramAnnotation.token; @@ -427,10 +425,6 @@ function _extractToken(typeOrFunc, annotations /*List | any*/, params: List } } - if (isBlank(visibility)) { - visibility = defaultVisibility; - } - token = resolveForwardRef(token); if (isPresent(token)) { @@ -439,16 +433,6 @@ function _extractToken(typeOrFunc, annotations /*List | any*/, params: List throw new NoAnnotationError(typeOrFunc, params); } } -function _defaulVisiblity(typeOrFunc) { - if (!(typeOrFunc instanceof Type)) return unbounded; - - // TODO: vsavkin revisit this after clarifying lookup rules - if (!reflector.isReflectionEnabled()) return unbounded; - - var f = - ListWrapper.filter(reflector.annotations(typeOrFunc), s => s instanceof InjectableMetadata); - return f.length === 0 ? unbounded : f[0].visibility; -} function _createDependency(token, optional, visibility, depProps): Dependency { return new Dependency(Key.get(token), optional, visibility, depProps); diff --git a/modules/angular2/src/di/decorators.dart b/modules/angular2/src/di/decorators.dart index 96efaab45a..7134628a07 100644 --- a/modules/angular2/src/di/decorators.dart +++ b/modules/angular2/src/di/decorators.dart @@ -21,7 +21,7 @@ class Optional extends OptionalMetadata { * {@link InjectableMetadata}. */ class Injectable extends InjectableMetadata { - const Injectable([VisibilityMetadata visibility = unbounded]): super(visibility); + const Injectable(): super(); } /** diff --git a/modules/angular2/src/di/decorators.ts b/modules/angular2/src/di/decorators.ts index 016e301526..84f50fe78c 100644 --- a/modules/angular2/src/di/decorators.ts +++ b/modules/angular2/src/di/decorators.ts @@ -30,8 +30,8 @@ export interface OptionalFactory { * Factory for creating {@link InjectableMetadata}. */ export interface InjectableFactory { - (visibility?: VisibilityMetadata): any; - new (visibility?: VisibilityMetadata): InjectableMetadata; + (): any; + new (): InjectableMetadata; } /** diff --git a/modules/angular2/src/di/injector.ts b/modules/angular2/src/di/injector.ts index 64ccf7f5de..caa6356953 100644 --- a/modules/angular2/src/di/injector.ts +++ b/modules/angular2/src/di/injector.ts @@ -14,7 +14,7 @@ import { import {FunctionWrapper, Type, isPresent, isBlank, CONST_EXPR} from 'angular2/src/facade/lang'; import {Key} from './key'; import {resolveForwardRef} from './forward_ref'; -import {VisibilityMetadata, unbounded} from './metadata'; +import {VisibilityMetadata, DEFAULT_VISIBILITY} from './metadata'; const _constructing = CONST_EXPR(new Object()); const _notFound = CONST_EXPR(new Object()); @@ -521,7 +521,9 @@ export class Injector { *binding). * @returns an instance represented by the token. Throws if not found. */ - get(token): any { return this._getByKey(Key.get(token), unbounded, false, PUBLIC_AND_PRIVATE); } + get(token): any { + return this._getByKey(Key.get(token), DEFAULT_VISIBILITY, false, PUBLIC_AND_PRIVATE); + } /** * Retrieves an instance from the injector. @@ -530,7 +532,7 @@ export class Injector { * @returns an instance represented by the token. Returns `null` if not found. */ getOptional(token): any { - return this._getByKey(Key.get(token), unbounded, true, PUBLIC_AND_PRIVATE); + return this._getByKey(Key.get(token), DEFAULT_VISIBILITY, true, PUBLIC_AND_PRIVATE); } /** diff --git a/modules/angular2/src/di/metadata.ts b/modules/angular2/src/di/metadata.ts index 50fcb3beaf..0fd6da2bae 100644 --- a/modules/angular2/src/di/metadata.ts +++ b/modules/angular2/src/di/metadata.ts @@ -79,7 +79,7 @@ export class DependencyMetadata { */ @CONST() export class InjectableMetadata { - constructor(public visibility: VisibilityMetadata = unbounded) {} + constructor() {} } /** @@ -123,8 +123,6 @@ export class SelfMetadata extends VisibilityMetadata { toString(): string { return `@Self()`; } } -export const self = CONST_EXPR(new SelfMetadata()); - /** * Specifies that an injector should retrieve a dependency from the direct parent. * @@ -235,4 +233,4 @@ export class UnboundedMetadata extends VisibilityMetadata { toString(): string { return `@Unbounded(self: ${this.includeSelf}})`; } } -export const unbounded = CONST_EXPR(new UnboundedMetadata({self: true})); \ No newline at end of file +export const DEFAULT_VISIBILITY = CONST_EXPR(new UnboundedMetadata({self: true})); \ No newline at end of file diff --git a/modules/angular2/test/core/compiler/element_injector_spec.ts b/modules/angular2/test/core/compiler/element_injector_spec.ts index 63678ec93c..93f100e14a 100644 --- a/modules/angular2/test/core/compiler/element_injector_spec.ts +++ b/modules/angular2/test/core/compiler/element_injector_spec.ts @@ -40,7 +40,7 @@ import { Directive, onDestroy } from 'angular2/annotations'; -import {bind, Injector, Binding, Optional, Inject, Injectable, Self, Parent, Ancestor, Unbounded, self, InjectMetadata, ParentMetadata} from 'angular2/di'; +import {bind, Injector, Binding, Optional, Inject, Injectable, Self, Parent, Ancestor, Unbounded, InjectMetadata, ParentMetadata} from 'angular2/di'; import {AppProtoView, AppView} from 'angular2/src/core/compiler/view'; import {ViewContainerRef} from 'angular2/src/core/compiler/view_container_ref'; import {ProtoViewRef} from 'angular2/src/core/compiler/view_ref'; @@ -73,16 +73,16 @@ class DummyElementRef extends SpyObject { noSuchMethod(m) { return super.noSuchMethod(m); } } -@Injectable(self) +@Injectable() class SimpleDirective {} class SimpleService {} -@Injectable(self) +@Injectable() class SomeOtherDirective {} var _constructionCount = 0; -@Injectable(self) +@Injectable() class CountingDirective { count; constructor() { @@ -91,7 +91,7 @@ class CountingDirective { } } -@Injectable(self) +@Injectable() class FancyCountingDirective extends CountingDirective { constructor() { super(); } } @@ -99,13 +99,13 @@ class FancyCountingDirective extends CountingDirective { @Injectable() class NeedsDirective { dependency: SimpleDirective; - constructor(dependency: SimpleDirective) { this.dependency = dependency; } + constructor(@Self() dependency: SimpleDirective) { this.dependency = dependency; } } @Injectable() class OptionallyNeedsDirective { dependency: SimpleDirective; - constructor(@Optional() dependency: SimpleDirective) { this.dependency = dependency; } + constructor(@Self() @Optional() dependency: SimpleDirective) { this.dependency = dependency; } } @Injectable() diff --git a/modules/angular2/test/di/injector_spec.ts b/modules/angular2/test/di/injector_spec.ts index c1c4269605..0b47c33b5d 100644 --- a/modules/angular2/test/di/injector_spec.ts +++ b/modules/angular2/test/di/injector_spec.ts @@ -16,9 +16,7 @@ import { forwardRef, DependencyMetadata, Injectable, - InjectMetadata, - self, - unbounded + InjectMetadata } from 'angular2/di'; import {InjectorInlineStrategy, InjectorDynamicStrategy} from 'angular2/src/di/injector'; @@ -29,15 +27,6 @@ class CustomDependencyMetadata extends DependencyMetadata {} class Engine {} -@Injectable(self) -class EngineWithSetVisibility { -} - -@Injectable() -class CarNeedsEngineWithSetVisibility { - constructor(engine: EngineWithSetVisibility) {} -} - class BrokenEngine { constructor() { throw new BaseException("Broken Engine"); } } @@ -417,19 +406,5 @@ export function main() { expect(binding.dependencies[0].properties).toEqual([new CustomDependencyMetadata()]); }); }); - - describe("default visibility", () => { - it("should use the provided visibility", () => { - var bindings = Injector.resolve([CarNeedsEngineWithSetVisibility, EngineWithSetVisibility]); - var carBinding = bindings[0]; - expect(carBinding.dependencies[0].visibility).toEqual(self); - }); - - it("should set the default visibility to unbounded", () => { - var bindings = Injector.resolve([Car, Engine]); - var carBinding = bindings[0]; - expect(carBinding.dependencies[0].visibility).toEqual(unbounded); - }); - }); }); }