-
Notifications
You must be signed in to change notification settings - Fork 27.5k
feat(getControllers): allow objects for require #8401
Conversation
in its current form, this is a breaking change. I also think it's a lot more verbose and thus a bit silly in some ways. I'd rather we leave this alone at least until we can properly support a version of |
} else if (isArray(require) || isObject(require)) {
value = isArray(require) ? [] : {}; it's very verbose but then so is using an Object for $Q.all. In my opinion I feel this bit of code is more clear if (ctrls.ngModelOptions) {
ctrls.ngModel.$options = ctrls.ngModelOptions.$options;
} than if (ctrls[2]) {
ctrls[0].$options = ctrls[2].$options;
} |
forEach(require, function(require) { | ||
value.push(getControllers(directiveName, require, $element, elementControllers)); | ||
value[key] = getControllers(directiveName, require, $element, elementControllers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you also forgot to add the key
parameter to the forEach callback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, yeah that's why you don't make PR's with github edits. I ran the unit tests locally with key
fix and had no errors
allowing objects, similar to $Q.all, read out more clearly for example current ngModelDirective ```javascript var ngModelDirective = function() { return { restrict: 'A', require: ['ngModel', '^?form', '^?ngModelOptions'], controller: NgModelController, link: { pre: function(scope, element, attr, ctrls) { // Pass the ng-model-options to the ng-model controller if (ctrls[2]) { ctrls[0].$options = ctrls[2].$options; } // notify others, especially parent forms var modelCtrl = ctrls[0], formCtrl = ctrls[1] || nullFormCtrl; formCtrl.$addControl(modelCtrl); scope.$on('$destroy', function() { formCtrl.$removeControl(modelCtrl); }); }, post: function(scope, element, attr, ctrls) { var modelCtrl = ctrls[0]; if (modelCtrl.$options && modelCtrl.$options.updateOn) { element.on(modelCtrl.$options.updateOn, function(ev) { scope.$apply(function() { modelCtrl.$$debounceViewValueCommit(ev && ev.type); }); }); } element.on('blur', function(ev) { scope.$evalAsync(function() { modelCtrl.$setTouched(); }); }); } } }; }; ``` ngModelDirective with a require object ```javascript var ngModelDirective = function() { return { restrict: 'A', require: { ngModel: 'ngModel', form: '^?form', ngModelOptions: '^?ngModelOptions' }, controller: NgModelController, link: { pre: function(scope, element, attr, ctrls) { // Pass the ng-model-options to the ng-model controller if (ctrls.ngModelOptions) { ctrls.ngModel.$options = ctrls.ngModelOptions.$options; } // notify others, especially parent forms var modelCtrl = ctrls.ngModel, formCtrl = ctrls.form || nullFormCtrl; formCtrl.$addControl(modelCtrl); scope.$on('$destroy', function() { formCtrl.$removeControl(modelCtrl); }); }, post: function(scope, element, attr, ctrls) { var modelCtrl = ctrls.ngModel; if (modelCtrl.$options && modelCtrl.$options.updateOn) { element.on(modelCtrl.$options.updateOn, function(ev) { scope.$apply(function() { modelCtrl.$$debounceViewValueCommit(ev && ev.type); }); }); } element.on('blur', function(ev) { scope.$evalAsync(function() { modelCtrl.$setTouched(); }); }); } } }; }; ```
I updated the PR with the missing |
anyways, I guess I don't have a huge problem with it if it's more convenient, but to me it really just looks like more typing. I'll triage this for 1.3 and we can figure out if it's something we want to enable |
I don't think this is especially valuable. @IgorMinar – thoughts? |
I think that it is an improvement but it needs a lot of work still - mainly tests and docs. |
I also think having a similar interface as the scope property could also be beneficial for less code. for example. var ngModelDirective = function() {
return {
restrict: 'A',
require: {
ngModel: true,
form: '^?',
ngModelOptions: '^?'
},
link: function(s, e, a, ctrls) {
/*
you would use a ctrl object
ctrls.ngModel
ctrls.form
ctrls.ngModelOptions
rather than
var ngModel = ctrls[0];
var formCtrl = ctrls[1];
var ngModelOptions = ctrls[2];
*/
} |
02dc2aa
to
fd2d6c0
Compare
cad9560
to
f294244
Compare
e8dc429
to
e83fab9
Compare
4dd5a20
to
998c61c
Compare
OK, let's get this into 1.5. Docs and tests needed. |
do you think someone from the community can take over for this PR? preferably someone new to open-source or new to contributing to the repo |
Sounds like a great idea @gdi2290 |
It's @gdi2290 actually 😉 |
That's what I said :-P |
allowing objects, similar to $Q.all, read out more clearly for example
current ngModelDirective
ngModelDirective with a require object