fix(Module): allow passing template/templateUrl in array notation #13485
Conversation
|
LGTM |
| @@ -362,7 +362,7 @@ function setupModuleLoader(window) { | |||
| component: function(name, options) { | |||
| function factory($injector) { | |||
| function makeInjectable(fn) { | |||
| if (angular.isFunction(fn)) { | |||
| if (isFunction(fn) || isArray(fn)) { | |||
There was a problem hiding this comment.
I am pretty sure we cannot rely on being in the closure of angular here.
Although the unit tests will pass, I don't think that isFunction and isArray will be available in practice.
There was a problem hiding this comment.
isFunction is used further down in the same file, so it means we're fine, no? https://github.com/angular/angular.js/blob/master/src/loader.js#L454
There was a problem hiding this comment.
I would like to see that working in practice before I agree... I wonder if that other use ever worked... :-)
There was a problem hiding this comment.
That usage was added by an external committer... 351fe4b
There was a problem hiding this comment.
If that line didn't work every single service/directive/etc. you define would throw... (btw, it also has a bug of not handling array notation 😄)
There was a problem hiding this comment.
if you just do angular.isArray you would break the loader as it is meant to be run without angular
There was a problem hiding this comment.
I think there's an open issue about the loader being broken for certain modules. Maybe that already happens?
There was a problem hiding this comment.
Array.isArray is supported on all the browsers that we support in (outside of v1.2) (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/isArray#Browser_compatibility) so should just use that in this case...
There was a problem hiding this comment.
We should definitely have a comment at the top of this file, explaining where we get isFunction from.
There was a problem hiding this comment.
@petebacondarwin in 99% of the cases (that is when people use angular and not just the loader), isFunction comes from src/Angular.js and I doubt that there is a need to explain this. For the 1% of cases that the loader is used stand-alone, then it comes from another place.
|
I feel like this should be documented, no? |
|
Also, what's the reason why component() itself is not injectable? |
|
|
|
I think it is nice that we are not injecting into the CDO. Anything that needs injection can ask for it. |
|
I'd still like to see a mention in the docs that you can you can inject any injectables into the template / templateUrl fn. This is far from obvious. |
|
@Narretz do you want to open an issue to track that? |
|
@lgalfaso I've added this to the docs just now, but I would really prefer if docs landed as part of the code changes. |
No description provided.