-
Notifications
You must be signed in to change notification settings - Fork 95
feat(injector): implement a different approach to injector scopes #50
base: master
Are you sure you want to change the base?
Conversation
This PR makes this test pass (discussed with vojta, having this test pass is not set in stone): it('should do stuff and work', function () {
class RouteScope extends ScopeAnnotation {}
function Compiler() {}
@Inject(Compiler)
function Foo(compiler) {
this.compiler = compiler;
}
@RouteScope
@Inject(Foo, Compiler)
function RequestHandler(foo, compiler) {
this.foo = foo;
this.compiler = compiler;
}
@Provide(Compiler)
function RouteCompiler() {}
var injector = new Injector(),
child = injector.createChild([RouteCompiler], [RouteScope]);
var reqHandler = child.get(RequestHandler);
expect(reqHandler.compiler).not.toBe(reqHandler.foo.compiler);
expect(reqHandler.compiler).toBeInstanceOf(RouteCompiler);
expect(reqHandler.foo.compiler).toBeInstanceOf(Compiler);
}); I'll amend the PR to add this test if that's the behavior that we want. |
@@ -146,6 +157,7 @@ export { | |||
readAnnotations, | |||
|
|||
SuperConstructor, | |||
ScopeAnnotation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's call it Scope
to make it consistent. Or DefaultScope
, BaseScope
or something?
I think you are right and we might rename all of the annotations to *Annotation
but that can be done in a separate commit. The reason why I didn't do that already was that then people have to import it as import {InjectAnnotation as Inject} from 'di';
... and that kind of sucks....
We should probably separate all these annotations to a separate module because often projects only need the annotations (so that their code can be used with DI) but they don't need the actual DI. Then, I think Inject
, Scope
, etc. will be fine...
What do you think?
@rodyhaddad this is awesome. Thank you! I left some subtle comments, nothing serious... |
it('should be instantiated by the root injector if a service doesn\'t have a scope', function () { | ||
function Service(){} | ||
|
||
var injector = new Injector(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this fancy indentation to:
var a = b;
var c = d;
var e = f;
I know we use this in Angular codebase, but I prefer this old-school style, it's much easier to add new var or re-order...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for other places....
e46889e
to
e4c8659
Compare
An easy way to review this is to read the tests and see if that's what we want