feat(directive-injector): detect and throw on circular deps #1364
Conversation
| @@ -52,6 +52,8 @@ const int CONTENT_PORT_KEY_ID = 16; | |||
| const int EVENT_HANDLER_KEY_ID = 17; | |||
| const int KEEP_ME_LAST = 18; | |||
|
|
|||
| final int MAX_RESOLVING_DEPTH = 50; | |||
There was a problem hiding this comment.
I would have used const, not sure if it makes any diff but at least it would be consistent with the const above
There was a problem hiding this comment.
According to the Dart docs "A final variable can be set only once; a const variable is a compile-time constant." Indeed const sounds stronger and more correct.
| @@ -122,6 +124,8 @@ class DirectiveInjector implements DirectiveBinder { | |||
| Key _key8 = null; dynamic _obj8; List<Key> _pKeys8; Function _factory8; | |||
| Key _key9 = null; dynamic _obj9; List<Key> _pKeys9; Function _factory9; | |||
|
|
|||
| int _resolving_depth; | |||
|
About the exception: can't you actually re-use |
|
With context change we would need some custom logic in it so that we can direct the user to ScopeAware if the circular dependency involves scope. So we can't use CircularDependencyError without some modifications. It is true that we can inherit from it, but in general I rather not tie up angular and di to much as they are separate packages and the implementation is rather trivial. |
|
Extending actually has an other benefit. If the directive injector fallback to the regular injector you don't need two different catch clauses. |
|
That's a valid point, by not catching di's CircDepError in directive injector if we have: |
|
@rkirov I'm not sure to get your last remark. Do you mean that the current is ok as we don't want to catch the DI:CircError ? |
|
Actually, I just noticed that we were already try-catching di::ResolutionError, so I integrated the new error with the same try-catch (but I had to move it). That should handle the case I was going to ignore initially. Also, I ran into problems of calls to _parent._getDirectiveByKey that skipped try-catch, so I had to do some modification (take a look at the code). There is a child-parentInjector error case added. |
|
@rkirov Please update this PR so the Travis tests pass. |
|
Rebased and modified. Significantly changed how it works. Now all modifications live inside _new, so it is clear this is enforcing a bound on construction, not resolution. I would like someone to take another look at this before merging. |
The maximum depth for depth for dependency construction within a single injector is set to 50. No performance degradation observed on tree benchmark.
The maximum depth for depth for dependency construction within a single injector is set to 50. No performance degradation observed on tree benchmark. Closes #1364
The maximum depth for depth for dependency resolution is set to 50.
No performance degradation observed on tree benchmark.