Skip to content
This repository has been archived by the owner on Apr 13, 2022. It is now read-only.

feat(getControllers): allow objects for require #8401

Closed
wants to merge 1 commit into from

Conversation

PatrickJS
Copy link
Member

allowing objects, similar to $Q.all, read out more clearly for example
current ngModelDirective

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

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();
          });
        });
      }
    }
  };
};

@caitp
Copy link
Contributor

caitp commented Jul 29, 2014

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 ^ which doesn't inspect the current element

@PatrickJS
Copy link
Member Author

I don't think it's breaking change since you still are able to use an array never mind travis failed to build

} 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);
Copy link
Contributor

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

Copy link
Member Author

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();
});
});
}
}
};
};
```
@PatrickJS
Copy link
Member Author

I updated the PR with the missing key argument. both unit tests and e2e are passing now

@caitp
Copy link
Contributor

caitp commented Jul 29, 2014

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

@btford
Copy link
Contributor

btford commented Aug 28, 2014

I don't think this is especially valuable. @IgorMinar – thoughts?

@IgorMinar
Copy link
Contributor

I think that it is an improvement but it needs a lot of work still - mainly tests and docs.

@PatrickJS
Copy link
Member Author

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];
     */

    }

@petebacondarwin
Copy link
Member

OK, let's get this into 1.5. Docs and tests needed.

@petebacondarwin petebacondarwin modified the milestones: 1.5.x - migration-facilitation, 1.3.x - superluminal-nudge Sep 7, 2015
@PatrickJS
Copy link
Member Author

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

@petebacondarwin
Copy link
Member

Sounds like a great idea @gdi2290

@gkalpak
Copy link
Member

gkalpak commented Nov 7, 2015

It's @gdi2290 actually 😉

@petebacondarwin
Copy link
Member

That's what I said :-P

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants