Skip to content
This repository was archived by the owner on Feb 26, 2024. It is now read-only.

feat(injector): implement a different approach to injector scopes #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rodyhaddad
Copy link
Contributor

An easy way to review this is to read the tests and see if that's what we want

Review on Reviewable

@rodyhaddad
Copy link
Contributor Author

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,
Copy link
Contributor

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?

@vojtajina
Copy link
Contributor

@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(),
Copy link
Contributor

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for other places....

@vojtajina vojtajina force-pushed the master branch 2 times, most recently from e46889e to e4c8659 Compare August 23, 2014 16:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants