Skip to content
This repository has been archived by the owner on Feb 22, 2018. It is now read-only.

The usually suggested way of binding components and decorators can lead to subtle bugs/unintended injections #1398

Open
BlackHC opened this issue Aug 27, 2014 · 1 comment
Milestone

Comments

@BlackHC
Copy link
Contributor

BlackHC commented Aug 27, 2014

Most of the Angular.dart code I've seen uses calls like: ..bind(ExampleDecorator) (A) to register components and decorators in the module and make them available during template expansion.
However, I just stumbled over some other code that uses ..bind(ExampleDecorator, toValue: null) (B).

This surprised me at first but the more I think about it the more it makes sense: By binding decorators the normal way (A), you allow di to create a default singleton instance. By using toValue: null (B), you prevent that.

The normal way (A) (as also suggested by the angular.dart tutorials) of doing this can lead to subtle bugs:
If you depend on the injection of a component or decorator that happens not to be there, (A) will cause the default instance to be injected and the code will access this global singleton instance instead of the local one that was expected. (B) will cause null to be injected which will cause an NPE on access at least. (I'm used to Guice, so I would have expected and preferred dependency injection to fail right away, but that is not that important.)

Especially for bigger applications, the currently suggested way of registering components and decorators can lead to bugs that are very hard to debug.

I see that toValue: null is also used internally by Angular.dart. Could the tutorials and documentation be updated to include a short discussion of (A) vs (B), or just use (B)?

Also, thinking about this some more, to me it feels like the current way of registering components and decorators using di modules does not seem very clean. We only want to register their type with angular.dart and never want to actually bind (singleton) instances of them to anything. Has this been discussed before?

Thanks,
Andreas

@mhevery
Copy link
Contributor

mhevery commented Oct 2, 2014

Directive registration should be split from injection system. This will be addressed in the upcoming compiler rewrite.

@naomiblack naomiblack modified the milestones: post v1.0, v1.0 Oct 3, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

3 participants