Skip to content

Commit

Permalink
fix(di): removed default visibility
Browse files Browse the repository at this point in the history
BREAKING CHANGE:
    Directives will use the Unbounded visibility by default, whereas before the change they used Self
  • Loading branch information
vsavkin committed Jul 13, 2015
1 parent 4bdc918 commit 04baa46
Show file tree
Hide file tree
Showing 10 changed files with 26 additions and 69 deletions.
3 changes: 1 addition & 2 deletions modules/angular2/di.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions modules/angular2/src/core/annotations_impl/annotations.ts
Original file line number Diff line number Diff line change
@@ -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';

/**
Expand Down Expand Up @@ -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;
Expand Down
3 changes: 1 addition & 2 deletions modules/angular2/src/core/compiler/element_injector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@ import {
CyclicDependencyError,
resolveForwardRef,
VisibilityMetadata,
DependencyProvider,
self
DependencyProvider
} from 'angular2/di';
import {
InjectorInlineStrategy,
Expand Down
24 changes: 4 additions & 20 deletions modules/angular2/src/di/binding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
InjectableMetadata,
VisibilityMetadata,
OptionalMetadata,
unbounded,
DEFAULT_VISIBILITY,
DependencyMetadata
} from './metadata';
import {NoAnnotationError} from './exceptions';
Expand All @@ -30,7 +30,7 @@ export class Dependency {
public properties: List<any>) {}

static fromKey(key: Key): Dependency {
return new Dependency(key, false, _defaulVisiblity(key.token), []);
return new Dependency(key, false, DEFAULT_VISIBILITY, []);
}
}

Expand Down Expand Up @@ -397,18 +397,16 @@ function _extractToken(typeOrFunc, annotations /*List<any> | 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;
Expand All @@ -427,10 +425,6 @@ function _extractToken(typeOrFunc, annotations /*List<any> | any*/, params: List
}
}

if (isBlank(visibility)) {
visibility = defaultVisibility;
}

token = resolveForwardRef(token);

if (isPresent(token)) {
Expand All @@ -439,16 +433,6 @@ function _extractToken(typeOrFunc, annotations /*List<any> | 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);
Expand Down
2 changes: 1 addition & 1 deletion modules/angular2/src/di/decorators.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class Optional extends OptionalMetadata {
* {@link InjectableMetadata}.
*/
class Injectable extends InjectableMetadata {
const Injectable([VisibilityMetadata visibility = unbounded]): super(visibility);
const Injectable(): super();
}

/**
Expand Down
4 changes: 2 additions & 2 deletions modules/angular2/src/di/decorators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down
8 changes: 5 additions & 3 deletions modules/angular2/src/di/injector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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.
Expand All @@ -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);
}

/**
Expand Down
6 changes: 2 additions & 4 deletions modules/angular2/src/di/metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export class DependencyMetadata {
*/
@CONST()
export class InjectableMetadata {
constructor(public visibility: VisibilityMetadata = unbounded) {}
constructor() {}
}

/**
Expand Down Expand Up @@ -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.
*
Expand Down Expand Up @@ -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}));
export const DEFAULT_VISIBILITY = CONST_EXPR(new UnboundedMetadata({self: true}));
14 changes: 7 additions & 7 deletions modules/angular2/test/core/compiler/element_injector_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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() {
Expand All @@ -91,21 +91,21 @@ class CountingDirective {
}
}

@Injectable(self)
@Injectable()
class FancyCountingDirective extends CountingDirective {
constructor() { super(); }
}

@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()
Expand Down
27 changes: 1 addition & 26 deletions modules/angular2/test/di/injector_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,7 @@ import {
forwardRef,
DependencyMetadata,
Injectable,
InjectMetadata,
self,
unbounded
InjectMetadata
} from 'angular2/di';

import {InjectorInlineStrategy, InjectorDynamicStrategy} from 'angular2/src/di/injector';
Expand All @@ -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"); }
}
Expand Down Expand Up @@ -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);
});
});
});
}

0 comments on commit 04baa46

Please sign in to comment.