Add a public "has" method to the Controller class, to check the existence of a given controller by Vadorequest · Pull Request #14109 · angular/angular.js

Vadorequest · 2016-02-22T22:04:00Z

I haven't written any test. The npm install failed here and I've been unable to test anything. (Seems like it fails on gyp-rebuild with my WIndows 8.1...)

The PR is quite small. if someone with a proper setup could just add some test for it, it would be nice.

#13951

googlebot · 2016-02-22T22:04:02Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

googlebot · 2016-02-22T22:04:02Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

gkalpak · 2016-02-23T08:53:27Z

      globals = false;

+  /**
+   * @ngdoc method


Use {...} instead of (...)

gkalpak · 2016-02-23T09:04:55Z

I left a couple of minor comments. There are also some styling issues that JSCS complains about (you can take a look here).

Could you also add Fixes #13951 at the end of your commit message ? This way, the related issue will be automatically closed when this PR is merged.

Regarding npm install:
Indeed gyp-rebuild will throw lots of read in your face, while trying to rebuild some modules on Windows. This doesn't necessarily mean the installation failed (it will just fall back to pre-built artifacts).
Are you sure npm install failed ? (Even if it has, the gyp-rebuild failures might not be the cause.)

Regarding tests:
Even if you are not able to run the tests locally, you could still add them and push the changes to this PR, so CI can run them for you. It's not necessary though (and it's tooo slow and obviously less than ideal 😃).

googlebot · 2016-02-23T10:11:56Z

CLAs look good, thanks!

googlebot · 2016-02-23T10:11:56Z

CLAs look good, thanks!

Vadorequest · 2016-02-23T10:17:55Z

@gkalpak Thanks for feedback. I've pushed changes. I'm installing the environment on an Ubunutu now. I don't know what happened about gyp-rebuild but many node modules weren't properly installed after the red messages, I assumed it didn't go well. :)

(I forgot about Fixes #13951 but since I'll eventually have to rebase -i I'll add it later anyway.

gkalpak · 2016-02-23T10:57:46Z

Great ! The code LGTM. Let us know if you run into any difficulties setting up the testing environment.

Vadorequest · 2016-02-23T11:02:02Z

I just pushed some tests, hope they'll pass. Not sure aboute the it messages though.
I definitely ran into issues with the setup, I got red messages again after a npm install, but all packages seemed to be installed, then the whole node_modules folder got deleted. I really don't get it. Seems like an exception has been thrown somewhere and the whole thing got cancelled, I tried twice, same result.

So, CI's got to do the testing part :)

gkalpak · 2016-02-23T11:30:36Z

Are you by any chance using npm >=3.0.0 ? That would explain the node_modules/ directory getting deleted after modules are installed. It's because the execution order of pre/post-install scripts wrt to other operations has changed in npm 3.
Our setup is currently not compatible with npm 3 (and even if you worked around that, I doubt all our dependencies are). This is tracked in #13505 btw.

gkalpak · 2016-02-23T11:33:11Z

There were some failures 😃
Taking a closer look...

gkalpak · 2016-02-23T11:41:12Z

+
+      $controllerProvider.register({FooCtrl: FooCtrl});
+      expect($controllerProvider.has('BarCtrl')).toBe(false);
+    });


I would basically merge both tests, into something like:

$controllerProvider.register('FooCtrl', noop); expect($controllerProvider.has('FooCtrl')).toBe(true); expect($controllerProvider.has('BarCtrl')).toBe(false);

Or if we wanted to be more thorough, something like:

$controllerProvider.register('FooCtrl', noop); $controllerProvider.register('BarCtrl', ['dep1', 'dep2', noop]); $controllerProvider.register({ 'BazCtrl': noop, 'QuxCtrl': ['dep1', 'dep2', noop] }); expect($controllerProvider.has('FooCtrl')).toBe(true); expect($controllerProvider.has('BarCtrl')).toBe(true); expect($controllerProvider.has('BazCtrl')).toBe(true); expect($controllerProvider.has('QuxCtrl')).toBe(true); expect($controllerProvider.has('UnknownCtrl')).toBe(false);

I don't understand what noop is here, should I replace it by {}?

Sorry, too much internals 😁
noop is basically angular.noop, which is basically function() {} (a no-op function). You can use noop as a shorthand for an empty constructor function. For this functionality, you don't need to pass any arguments to the controller, nor does the controller need to have any logic.

gkalpak · 2016-02-23T11:41:48Z

@Vadorequest, do you think it would be useful to have the method available on $controller as well ?

Vadorequest · 2016-02-23T11:45:08Z

@gkalpak No, I don't think the controller needs to have a has method, the provider makes much more sense.

Vadorequest · 2016-02-23T12:02:02Z

Tests passed :) Cool.

And yes, I'm using node 5.5, that's why it fails during the setup.

Vadorequest · 2016-02-23T13:24:28Z

Using node 3.3.1 it worked well, I've been able to run the tests and everything looks good.
The PR has been squashed and is ready to be merged.

gkalpak · 2016-02-23T20:49:17Z


+    it('should return true when having an existing controller, should return false otherwise', function() {
+      var FooCtrl = function($scope) { $scope.foo = 'foo'; },
+          BarCtrl = function($scope) { $scope.bar = 'bar'; };


We don't need these two controllers, do we ?

Indeed! I just removed them.

gkalpak · 2016-02-23T20:52:01Z

@Vadorequest, only a minor clean-up comment. LGTM otherwise 👍

…ss, to check the existence of a given controller. Fixes angular#13951

…tence of a controller Fixes #13951 Closes #14109

gkalpak · 2016-02-23T22:02:27Z

Thx @Vadorequest 👍
Backported to v1.5.x as bb9575d.

Vadorequest · 2016-02-23T22:27:35Z

Thanks to you @gkalpak :)
Does that mean that it can be used in the latest version righ now?

gkalpak · 2016-02-23T22:44:41Z

It will be available from the upcoming v1.5.1 release onwards. Right now you can try it out with the latest snapshot, which is basically the code currently on master.
(NOTE: The snapshot changes with every commit to master and might contain breaking changes. It is not recommened for use in production 😃 - it's just an easy way to try something out before it's released.)

Vadorequest · 2016-02-23T22:45:52Z

Thanks ;)

googlebot added the cla: no label Feb 22, 2016

gkalpak reviewed Feb 23, 2016
View reviewed changes

Comment thread src/ng/controller.js

globals = false;

/**

* @ngdoc method

Copy link
Copy Markdown

Member

gkalpak Feb 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use {...} instead of (...)

gkalpak added type: feature frequency: moderate needs: work component: misc core needs: test severity: inconvenient labels Feb 23, 2016

gkalpak added this to the 1.5.x milestone Feb 23, 2016

googlebot added cla: yes and removed cla: no labels Feb 23, 2016

gkalpak reviewed Feb 23, 2016
View reviewed changes

Vadorequest force-pushed the controller-has branch from 56854da to 11a9111 Compare February 23, 2016 13:23

Vadorequest changed the title ~~[NEED TESTS] Add a public "has" method to the Controller class, to check the existence of a given controller~~ Add a public "has" method to the Controller class, to check the existence of a given controller Feb 23, 2016

gkalpak reviewed Feb 23, 2016
View reviewed changes

gkalpak removed the needs: test label Feb 23, 2016

core(controller.has): Add a public "has" method to the Controller cla…

bcaa151

…ss, to check the existence of a given controller. Fixes angular#13951

Vadorequest force-pushed the controller-has branch from 11a9111 to bcaa151 Compare February 23, 2016 21:21

gkalpak closed this in 76f47d5 Feb 23, 2016

gkalpak pushed a commit that referenced this pull request Feb 23, 2016

feat($controllerProvider): add a has() method for checking the exis…

bb9575d

…tence of a controller Fixes #13951 Closes #14109

Conversation

Vadorequest commented Feb 22, 2016

Uh oh!

googlebot commented Feb 22, 2016

Uh oh!

googlebot commented Feb 22, 2016

Uh oh!

gkalpak Feb 23, 2016

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Feb 23, 2016

Uh oh!

googlebot commented Feb 23, 2016

Uh oh!

googlebot commented Feb 23, 2016

Uh oh!

Vadorequest commented Feb 23, 2016

Uh oh!

gkalpak commented Feb 23, 2016

Uh oh!

Vadorequest commented Feb 23, 2016

Uh oh!

gkalpak commented Feb 23, 2016

Uh oh!

gkalpak commented Feb 23, 2016

Uh oh!

gkalpak Feb 23, 2016

Choose a reason for hiding this comment

Uh oh!

Vadorequest Feb 23, 2016

Choose a reason for hiding this comment

Uh oh!

gkalpak Feb 23, 2016

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Feb 23, 2016

Uh oh!

Vadorequest commented Feb 23, 2016

Uh oh!

Vadorequest commented Feb 23, 2016

Uh oh!

Vadorequest commented Feb 23, 2016

Uh oh!

gkalpak Feb 23, 2016

Choose a reason for hiding this comment

Uh oh!

Vadorequest Feb 23, 2016

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Feb 23, 2016

Uh oh!

gkalpak commented Feb 23, 2016

Uh oh!

Vadorequest commented Feb 23, 2016

Uh oh!

gkalpak commented Feb 23, 2016

Uh oh!

Vadorequest commented Feb 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants