respect explicit return values from controller functions (fixes #11147)

[jamestalmage]

When controller functions return an explicit value that value should be what is passed to the linking functions, and to any child/sibling controllers that require it. It should also be bound to the data store on the DOM element.

Closes #11147

[googlebot]

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

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 the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

[jamestalmage]

CLA updated

[googlebot]

CLAs look good, thanks!

[petebacondarwin]

@caitp you have done some work on this part of the code. Can you take a look at this one?

[caitp]

the only thing that's needed here is updating the element data. this is a real bug since it affects the behaviour of require'd directives :(

[caitp]

in particular, don't worry about the isObject() || isFunction() stuff, since that's handled in controller.js anyways

[jamestalmage]

Nevermind
I'm not sure what you are asking for here. We should mimic the behavior of native javascript: If they return an object or function use that as the result of the instantiation, otherwise return "as normal":

function returnsStr() {
  this.foo = "bar";
  return "baz";
}

new returnsStr() === {foo:"bar"} // "baz" return value is ignored

[caitp]

this code is duplicating code which already runs in the controller() function itself, it's not needed here

[caitp]

particularly here (the instantiate object is not updated here, because the compiler needed to be able to determine if a new result was returned so that the bindings could be reset --- so you still need to set controller.instance)

[31812015]

this diff

[jamestalmage]

Nevermind,
I see what you are saying, my above concern is already handled by the call to the controller() callback. I am adding a test case that verifies it.

[jamestalmage]

argh!
Come on Github! Real time updates for line comments!

[caitp]

LGTM with comments addressed

[jamestalmage]

unclear what this means

the only thing that's needed here is updating the element data

Are you asking me to delete the first of these two lines?:

controller.instance = controllerResult;
$element.data('$' + directive.name + 'Controller', controllerResult);

Both lines are required for my test to pass.

[caitp]

no, you need that line as mentioned above --- I would like you to add additional tests, though.

1. add another test where a controller which returns a custom object is require'd by another directive, assert that the directive gets the right controller (this is sort of what you're testing already, but it tests what we actually want, rather than the mechanics of it)
2. add similar test cases which verify the transclude cases

[jamestalmage]

`2. add similar test cases which verify the transclude cases

your use of plural case(s) is sending me down a rabbit hole in compileSpec. There are many tests related to transclusion and controllers.

I've added this one so far:

    it('should respect controller return value when using controllerAs', function() {
      module(function() {
        directive('main', function() {
          return {
            templateUrl: 'main.html',
            transclude: true,
            scope: {},
            controller: function() {
              this.name = 'lucas';
              return {name: 'george'};
            },
            controllerAs: 'mainCtrl'
          };
        });
      });
      inject(function($templateCache, $compile, $rootScope) {
        $templateCache.put('main.html', '<span>template:{{mainCtrl.name}} <div ng-transclude></div></span>');
        element = $compile('<div main>transclude:{{mainCtrl.name}}</div>')($rootScope);
        $rootScope.$apply();
        expect(element.text()).toBe('template:george transclude:');
      });
    });

But there are plenty of other cases. You want me to go nuts and add dozens of test cases, or are there specific scenarios you want tested?

[caitp]

you don't need "dozens" of tests, but add ones that deal with a directive whose controller gets the transclude function

[jamestalmage]

@caitp I believe I have addressed all of your concerns, but I am not familiar enough with the inner workings of transclude to be sure I have covered all cases. Please let me know if there is a scenario I have missed (pointers to existing tests would be helpful).

[caitp]

we don't need to actually run the transclude fn, we just need to assert that it's there

[caitp]

It would be good to make these test cases a little closer, so that the only difference between them is the presence of the transclude property of the DDO. almost there =)

[jamestalmage]

so proving each clone gets the correct object reference to the parent controller has no value? (serious question)

[caitp]

no, this is tested elsewhere --- we only really care that the nested directive gets the right controller

[jamestalmage]

in my mind
"each clone gets the correct object reference to the parent controller" === "the nested directive gets the right controller".

I need to call $transclude at least once to see what the nested controller gets, right?

[caitp]

well to link the nested directive yes, but you don't need to call it 3 times --- an expect(controller).toBe(the right thing) is the only real assertion that is needed

[jamestalmage]

understood.
on it.

[jamestalmage]

@caitp, made most of the changes requested, except this one:

It would be good to make these test cases a little closer, so that the only difference between them is the presence of the transclude property of the DDO

Can you clarify which tests you want to look closer?

[caitp]

merged, thanks for the work

[jamestalmage]

awesome. Thank you!