dart-archive / di.dart Goto Github PK

I open this issue following a discussion on PR 135

When a ModuleInjector is created, we create two lists for bindings and instances. Those two list have as many elements as the number of created keys at the time the injector is created (numInstance is auto-increment).

List have been preferred over Maps because they are faster on micro benchmarks.

IMO using lists could be a problem on real-life application (vs micro-benchmarks). If the application create keys and injectors, the memory consumption for those lists will keep increasing and is not bounded to a upper limit.

Using an map (HashMap) would be more efficient in term of memory consumption even if some cycles are lost on micro-benchmarks.

(More generally I think we should be more careful on conclusions drawn from micro-benchmarks)

Thoughts ?

/cc @mhevery @jbdeboer @antingshen @pavelgj

After running the generator for my project, I had this line in di_factories_gen.dart:

import "/home/myuser/source/dart/angular.dart.ui.demo/web/index.dart" as import_0;

I changed it to

import "index.dart" as import_0;

The application worked with both paths in Dartium and with pub build.

The generator should not add the absolute path anyway.

Issue is that web developers should be using either mirrors or static injector, but the two differ in how they handle types at runtime.

There should be some way to enforce that the dynamic injector and static injector have similar runtime behavior.

Currently, factories need to create a new Key to look up their dependencies. Instead, the type factories should use static keys.

@antingshen is working on this issue.

see #53

I'll probably experiment next week unless someone tackles this before

• Is every injection singlton based of is a new one created each time?
• is toFactory called each time or is that singleton based aswell?

Right now an error like this is produces when default constructor for an injectable type is not found by DynamicInjector.

Uncaught TypeError: Cannot call method 'get$parameters' of undefined dynamic_injector.dart:42
DynamicInjector.newInstanceOf$4 dynamic_injector.dart:42
_TypeProvider.get$4 module.dart:142
Injector__getInstanceByType_closure.call$0 injector.dart:118
_defaultCreationStrategy module.dart:106
Injector._getInstanceByType$2 injector.dart:116
(anonymous function) VM7762:3
DynamicInjector_newInstanceOf_resolveArgument.call$1 dynamic_injector.dart:39
List_List$generate list.dart:127
DynamicInjector.newInstanceOf$4 dynamic_injector.dart:42
_TypeProvider.get$4 module.dart:142
Injector__getInstanceByType_closure.call$0 injector.dart:118
...

class ParentDynamicallyInstantiatingChild {
  ParentDynamicallyInstantiatingChild(Injector injector) {
    injector.get(ChildDependingOnParent);
  }
}

class ChildDependingOnParent {
  ChildDependingOnParent(ParentDynamicallyInstantiatingChild _);
}

it('should be able through injector instantiate a class that depends on the class being instantiated', () {
  var injector = new Injector();
  expect(() {
    injector.get(ParentDynamicallyInstantiatingChild);
  }, not(throwsA(anything)));
});

throws
Cannot resolve a circular dependency! (resolving ParentDynamicallyInstantiatingChild -> ChildDependingOnParent -> ParentDynamicallyInstantiatingChild)

Current API:

value(Foo, new Foo(), withAnnotation: a, visibility: v);
factory(Foo, (i) => new Foo(), withAnnotation: a, visibility: v);
type(Foo, withAnnotation: a, visibility: v);
type(Foo, implementedBy: FooImpl, withAnnotation: a, visibility: v);

I think this syntax would be nicer on the eyes.

bind(Foo, annotatedWith: a, toValue: new Foo(), , visibility: v);
bind(Foo, annotatedWith: a, toFactory: (i) => new Foo(), visibility: v);
bind(Foo, annotatedWith: a, visibility: v);
bind(Foo, annotatedWith: a, toImplementation: FooImpl, visibility: v);

The current factory provider passes as argument to the FactoryFn the injector that contains the FactoryFn definition itself. We need another kind of factory that passes to the FactoryFn the requesting injector. The use cases come from AngularDart; details are provided below.

Use Case - AngularDart NgValue, NgTrueValue, NgFalseValue and more.

To keep things simple I will talk about NgValue here; the other cases are similar.
The ng-value directive is a subordinate directive that will always appear as an attribute of a select or input[radio].

Currently in the AngularDart NgDirectiveModule, the NgValue directive is added as follows:

class NgDirectiveModule extends Module {
  NgDirectiveModule() {
    ...
    value(NgValue, new NgValue(null)); // (1)
    ...
}

Let us consider the case of input[radio] elements and look at the directive's constructor:

  InputRadioDirective(dom.Element this.radioButtonElement, this.ngModel,
                      this.scope, this.ngValue, NodeAttrs attrs) { ... }

If a select or radio element does not have an ng-value attribute then the line marked (1) above will cause the DI to provide the same NgValue instance to all radio (and select) directives: the instance created by the call to the constructor at (1). Notice that the argument to the constructor invocation new NgValue(null) is null. The signature of the NgValue constructor is:

  NgValue(this.element);

Thus, what is happening is that the NgValue element field is set to null: as if we were saying that this NgValue is not a part of any element (but this is never the case). This is useless and it complicates the design of NgValue and the code for using it. E.g., here is part of the code of NgValue:

@NgDirective(selector: '[ng-value]')
class NgValue {
  final dom.Element element;
  @NgOneWay('ng-value')
  var value;

  NgValue(this.element);

  readValue(dom.Element element) {
    assert(this.element == null || element == this.element);
    return this.element == null || value == null 
        ? (element as dynamic).value : value;
  }
}

Notice how the design of readValue() is complicated by the fact that the top-level injected NgValue is shared (in which case element is null). Also, this design violates the DRY principle because the element value has to be passed as an argument to readValue() even if the field already holds this value.

Intuitively, it would seem that a DI factory provider should be what we need, thus a first attempt at solving our problem would be to rewrite the line (1) as:

    factory(NgValue, (Injector i) => new NgValue(i.get(dom.Element)));

Unfortunately, this does not work because the Injector argument i that is passed to the FactoryFn is the root injector. What we want is for the FactoryFn to be provide with the injector of the select or input[radio] so that each select and radio element effectively gets its own NgValue instance (which is what a factory should be doing).

With such a factory available, the code of NgValue.readValue() simplifies to:

  readValue() => value == null ? (element as dynamic).value : value;

This is a conceptually cleaner design. The improvements to NgTrueValue and NgFalseValue are event greater.

Mirrors are super slow. Let's try to have two modes:

• runtime (the current mode)
• generated code (similar to Dagger in Java)

The idea is to bind a type with generic, i.e. List to something (value/factory,..)

bind(List<num>, toValue: managedTypes, withAnnotation: ManagedType);

One way to work around this it to introduce type

class TypeLiteral<T> {
  Type get type => T;
}

and then bind:

bind(new TypeLiteral<List<num>>().type, withAnnotation: ManagedType, toValue: [1, 2, 3] ))

  factory Key(Type type, [Type annotation]) {
    var _hashCode = type.hashCode + annotation.hashCode;
    var _id = _hashToKey.putIfAbsent(_hashCode, () => _lastKeyId++);
    return new Key._newKey(type, annotation, _hashCode, _id);
  }

  Key._newKey(this.type, this.annotation, this.hashCode, this.id);

  bool operator ==(other) =>
      other is Key && other.hashCode == hashCode;

Is this code right ?

I think Key(A, B) == Key (B, A) is wrong.

Two objects that are equal have the same hash code but two objects with the same hash code are not necessary equal (however this is assumed by the current code)

key.uid is marked as @deprecated without further information.

However we do rely on uid being available in angular.

When is that supposed to be removed ? Any idea on how to deal without uid ?

/cc @antingshen @mhevery

This change suggested by James somehow got left in there after v2. Just need to remove that one line. Angular should already be updated to import it directly when needed.

This chunk of code is causing a really obscure error to be thrown:

MethodMirror ctor = classMirror.declarations[classMirror.simpleName];

resolveArgument(int pos) {
  ParameterMirror p = ctor.parameters[pos];
  try {
    return getInstanceBySymbol(p.type.qualifiedName);
  } on NoProviderError catch (e) {
    throw new NoProviderError(e.message + (isJs ? '' : ' at position $pos source:\n ${ctor.source}.'));
  }
}

In every scenario where a NoProviderError has been thrown (for me, at least), ctor.source has been null. When this is the case, MirrorsImpl fails an assertion: 'dart:mirrors-patch/mirrors_impl.dart': Failed assertion: line 1203: '_source != null' is not true.

They should run!

There may be a regression in DI 2.0.0 where DI fails to bind a type with an optional, named, typedef parameter.

e.g.

typedef void TD(String);
class A { A({TD td}); }

module.bind(A);

should work; and appears to have worked in DI 1

The primary task here is to write tests against DI 1 demonstrating this case. Then, rebase v2 to include those tests. Test that:
-> injecting classes with optional parameters works
-> injecting classes with named parameters works
-> injecting classes with a optional typedef parameter works
-> injecting classes with a named typedef parameter works

What about switching to a semver scheme.

0.0.40 has recently been release to fix 0.0.39.

It would be better to have switched from 0.m.0 to 0.m.1

It would be easier to track breaking changes for the users.

Add optional parameter to source crawler to allow generators to see doc comments. Useful for generators to generate documentation from the source.

e.g.

class A { A(B c, C c); }

main() {
module.bind(A);
}

gives the error:

Unhandled exception:
Closure call with mismatched arguments: function 'main.<anonymous closure>'

NoSuchMethodError: incorrect number of arguments passed to method named 'main.    <anonymous closure>'
Receiver: Closure: (dynamic) => dynamic
Tried calling: main.<anonymous closure>()
Found: main.<anonymous closure>(i)
#0      Object.noSuchMethod (dart:core-patch/object_patch.dart:45)
#1      ModuleInjector.getByKey (package:di/src/injector.dart:146:52)
#2      ModuleInjector.getByKey (package:di/src/injector.dart:129:42)
#3      Injector.get (package:di/src/injector.dart:31:18)
#4      main

This works

module.factory(String, (Injector i) => '');

But this does not

class MyFactory{
  call(Injector injector) => '';
}
module.factory(String, new MyFactory());

The issue is that the DI system does not recognize that the emulated functions should be treated as functions.

see dart-archive/angular.dart#961

The problem seems to be in dynamic_injector.newInstanceOf() which has an infinite recursion in some cases.

Example:

new Module()
  ..bind(Foo, toFactory: ...)
  ..bind(SuperFoo, toImplementation: Foo);

This used to work. Perhaps the new way is the correct one, because we also have inject: [Foo] that accomplishes the same goal. Maybe it's a matter of clarifying what toImplementation means.

Pub get failed, [1] Resolving dependencies...
Incompatible version constraints on code_transformers:
- di 2.0.1 depends on version >=0.1.4+2 <0.2.0
- polymer 0.12.0+7 depends on version >=0.2.0 <0.3.0

Are there plans to release a di version which supports code_transformer >=0.2.0+3?

For development is use

dependency_overrides:
  code_transformers: '>=0.2.0 <0.3.0'

but that prevents publishing the package to pub.dartlang.org

The new syntax is:

      ..bind(ElectricCar)
      ..bind(Engine, toFactory: (i) => new V8Engine())
      ..bind(Engine, toImplementation: ElectricEngine, withAnnotation: Electric)

I don't really like to have so many optional (and exclusive) args.

I would prefer

      ..bind(ElectricCar)
      ..bind(Engine).toFactory((i) => new V8Engine())
      ..bind(Engine).withAnnotation(Electric).toType(ElectricEngine)
      ..bind(Price).toValue(50000)

I can work on the implementation.

@pavelgj what do you think ?

lib1.dart

library lib1;

class Foo {}

lib2.dart

library lib2;

class Foo {}

main.dart

import 'package:di/di.dart';

import 'lib1.dart' as lib1;
import 'lib2.dart' as lib2;

main() {
  it('should distinguish between same Symbols from different libraries', () {
    var module = new Module()
      ..value(lib1.Foo, new lib1.Foo())
      ..value(lib2.Foo, new lib2.Foo());
    var injector = new Injector([module]);
    expect(injector.get(lib1.Foo), new isInstanceOf<lib1.Foo>());
    expect(injector.get(lib2.Foo), new isInstanceOf<lib2.Foo>()); // Fails
  });
}

Converting _providers to a List sped up instance creation; converting _instances should speed it up even further.

• Add a benchmark to the suite showing that _instances is slow
• Convert _instances to a List

evidently, primitive types are not allowed as keys to which you can bind values:

var module = new Module()..value(int, 3);
var injector = new DynamicInjector(modules: [module]);
injector.get(int); // Exception: Illegal argument(s): Cannot inject a primitive type of int! (resolving int)

but it would be nicer to have the exception earlier, like when the module is created,
rather than when the injector attempts to get the value.

See also http://stackoverflow.com/questions/21244644/how-can-i-dependency-inject-based-on-type-and-name-with-angulardart/21265773#21265773

In the case of two components each needing a unique instance of a shared type, I need a way to differentiate the injection. Using type alone is not enough.

to be consistent with dart2js output

The current Module class should be re-named ModuleInterface

di.dart should implement Module with the Dynamic reflector

the transformer should create a static Module with the static reflector.

the transformer should rewrite import 'di.dart' -> import 'di.dart', import 'di_static_module.dart'`

The null object does not have a getter 'simpleName'.

NoSuchMethodError : method not found: 'simpleName'
Receiver: null
Arguments: []

STACKTRACE:
#0 Object.noSuchMethod (dart:core-patch/object_patch.dart:45)
#1 getSymbolSimpleName (package:di/mirrors.dart:46:62)
#2 DynamicInjector._getProviderForSymbol (package:di/dynamic_injector.dart:154:33)
#3 DynamicInjector._getProviderForSymbol (package:di/dynamic_injector.dart:149:42)
#4 _getInstanceBySymbol (package:di/dynamic_injector.dart:92:53)
#5 DynamicInjector._wrapGetInstanceBySymbol. (package:di/dynamic_injector.dart:138:33)
#6 _TypeProvider.get.resolveArgument (package:di/dynamic_injector.dart:286:35)
#7 _TypeProvider.get.resolveArgument (package:di/dynamic_injector.dart:287:9)
#8 List.List.generate (dart:core/list.dart:127)
#9 _TypeProvider.get (package:di/dynamic_injector.dart:292:20)
#10 DynamicInjector._getInstanceBySymbol. (package:di/dynamic_injector.dart:117:40)
#11 _defaultCreationStrategy (package:di/module.dart:93:34)
#12 DynamicInjector._getInstanceBySymbol (package:di/dynamic_injector.dart:114:48)
#13 DynamicInjector._getInstanceBySymbol (package:di/dynamic_injector.dart:123:7)
#14 DynamicInjector.get (package:di/dynamic_injector.dart:185:27)
#15 BlockFactory._instantiateDirectives. (package:angular/core_dom/block_factory.dart:195:42)
#16 BlockFactory._instantiateDirectives. (package:angular/core_dom/block_factory.dart:217:9)
#17 BlockFactory._instantiateDirectives. (package:angular/core_dom/block_factory.dart:217:9)
#18 List.forEach (dart:core-patch/growable_array.dart:240)
#19 BlockFactory._instantiateDirectives (package:angular/core_dom/block_factory.dart:190:26)
#20 BlockFactory._link (package:angular/core_dom/block_factory.dart:83:51)
#21 BlockFactory._link (package:angular/core_dom/block_factory.dart:94:9)
#22 BlockFactory._link (package:angular/core_dom/block_factory.dart:94:9)
#23 BlockFactory._link (package:angular/core_dom/block_factory.dart:87:16)
#24 BlockFactory._link (package:angular/core_dom/block_factory.dart:94:9)
#25 BlockFactory._link (package:angular/core_dom/block_factory.dart:94:9)
#26 BlockFactory.call (package:angular/core_dom/block_factory.dart:51:12)
#27 BlockFactory.call (package:angular/core_dom/block_factory.dart:53:7)
#28 BlockFactory.call (package:angular/core_dom/block_factory.dart:53:7)
#29 ngBootstrap. (package:angular/bootstrap.dart:85:41)
#30 _rootRun (dart:async/zone.dart:688)
#31 _rootRun (dart:async/zone.dart:689)
#32 _rootRun (dart:async/zone.dart:689)
#33 _ZoneDelegate.run (dart:async/zone.dart:417)
#34 _onRun. (package:angular/core/zone.dart:62:63)
#35 NgZone._onRunBase (package:angular/core/zone.dart:49:16)
#36 _onRun (package:angular/core/zone.dart:62:22)
#37 _ZoneDelegate.run (dart:async/zone.dart:417)
#38 _CustomizedZone.run (dart:async/zone.dart:627)
#39 NgZone.run (package:angular/core/zone.dart:148:21)
#40 ngBootstrap (package:angular/bootstrap.dart:82:18)
#41 main (http://127.0.0.1:3030/bootstrap_angular/example/index.dart:24:14)

I turned out that I had an argument in the constructor of a custom controller that was missing the type annotation.

final _modal;
instead of

final Modal _modal
To locate the problem I set a breakpoint at line
#0 Object.noSuchMethod (dart:core-patch/object_patch.dart:45)

when the breakpoint hit, the following line in the callstack showed which argument was processed
#8 List.List.generate (dart:core/list.dart:127)

while at this line in the callstack the variable typeName showed which type Angular actually tried to instantiate
#9 _TypeProvider.get (package:di/dynamic_injector.dart:292:20)

Currently the following example fails, but should be able to work:

import 'package:di/di.dart';
import 'package:di/dynamic_injector.dart';

typedef String helloFn(String name);
String myFn(name) => "Hello World $name";

void main() {
  var m = new Module()
    ..value(helloFn, myFn);

  Injector i = new DynamicInjector(modules: [m]);
  // Note that this example works up until the type is resolved
  print(i.get(helloFn)("x"));
}

Using implementedBy should (theoretically) work:

new Module()
    ..type(helloFn, implementedBy: myFn);

And factories that provide functions should also work:

new Module()
    ..factory(helloFn, (_) => myFn);

There are a lot of cases where I want to hook up optional parameters that I want to use if provided, but plan to safely ignore it if null is passed or if the value is not configured. I don't want to force someone to configure an injectable value for every parameter.

As an example:

MyComponent(Param1 this.param1, Param2 this.param2, { Profiler profiler });

Instead of:

MyComponent(Param1 this.param1, Param2 this.param2, Profiler profiler);

Since providing null will throw warnings / and or exceptions for a required dependency, but making it optional (and/or named) implies that I understand that a value may not be provided.

It is common for apps to initialize certain objects asynchronously, way after the DI is configured. It would be really great to be able to inject a Future instead of the object instance itself.

abstract class BaseObject /*with GuiAttributes*/ {
  Map properties = {
    "top": 0.0,
    "left": 0.0
  };
}

class RectObject extends BaseObject {
  // width, height
}

class ImageObject extends RectObject {
  RectObject rect;

  ImageObject(this.rect);
}

class CircleObject extends BaseObject {
  // radius
}

abstract class GuiAttributes {
  bool showProperties = true;
}

class ExtendedRectObject = RectObject with GuiAttributes;
class ExtendedCircleObject = CircleObject with GuiAttributes;
class ExtendedImageObject = ImageObject with GuiAttributes;

bind(CircleObject, toImplementation: ExtendedCircleObject);
bind(RectObject, toImplementation: ExtendedRectObject);
bind(ImageObject, toImplementation: ExtendedImageObject);

injector.get(ImageObject)

leads to this stack trace

/home/zoechi/source/3rdparty/google/darteditor/dart-sdk/bin/dart --enable-checked-mode --debug:39396 main.dart
Breaking on exception: object of type NoProviderError
Unhandled exception:
Illegal argument(s): The 'rect' parameter must be typed (resolving ImageObject)
#0      DynamicInjector.newInstanceOf.resolveArgument (package:di/dynamic_injector.dart:47:9)
#1      List.List.generate (dart:core/list.dart:122)
#2      DynamicInjector.newInstanceOf (package:di/dynamic_injector.dart:68:20)
#3      TypeProvider.get (package:di/src/provider.dart:36:34)
#4      BaseInjector.getInstanceByKey (package:di/src/base_injector.dart:117:29)
#5      BaseInjector.get (package:di/src/base_injector.dart:158:23)
#6      main (file:///home/zoechi/source/my/dart/playground/bin/di/main.dart:67:45)
#7      _startIsolate.isolateStartHandler (dart:isolate-patch/isolate_patch.dart:216)
#8      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:124)

Given this module definition:

type(MockHttpBackend);
factory(HttpBackend, (Injector i) => i.get(MockHttpBackend));

When I call

injector.get(MockHttpBackend);
injector.get(HttpBackend);

I get MockHttpBackend instantiated twice.

The angular.dart tutorial states that:

@Injectables annotation should be mainly used with classes that are out of your control (ex. you can't modify the source code -- third party library). In all other cases it's preferable to use custom class annotation(s).

I'm really confused by this. Firstly - why two different ways to do the same thing? And second, the preferred way seems... weird. I have to create a custom annotation and then configure the generator to use it?

This should be an error.

new Module()
  ..value(String, null)
  ..value(String, '');

There is no reason why within a single module one should be able to give two different definitions to the same type, and is most likely an error and should be reported.

Allow a generic type to be injected without having to specify all possibly generic arguments. For example:

 bind(GenericType);

 class OtherType{
      OtherType(GenericType<String>){

      }
 }

I was curious how to solve the problem described here http://stackoverflow.com/questions/21943550

In the issues #48 and #51 you discussed that it should not be prevented to override a binding (I don't understand why you want to prevent this).
So this seems not to be a solution for the SO question.

How can custom default headers be set then?