fix($compile): be more careful about shadowing $attrs values

[caitp]

Shadows only when attributes are non-optional and not own properties,
only stores the observed '@' values when they are indeed strings.

Partial revert of a6339d3

Closes #12144

[petebacondarwin]

It is also true that this affects the isolated scope as well as the bindToController:

  it('should only observe attributes that are defined', function() {
      module(function($provide) {
        directive('missingInterpolatedAttrDirective', function() {
          return {
            restrict: 'E',
            scope: { prop: '@' },
            bindToController: { prop: '@' },
            controllerAs: 'vm',
            controller: function($scope) {
              this.prop = this.prop || 'init';
              $scope.prop = $scope.prop || 'init';
            },
            template: '<div>{{ prop }} | {{ vm.prop }}</div>'
          };
        });
      })
      inject(function($rootScope, $compile) {
        element = $compile('<missing-interpolated-attr-directive></missing-interpolated-attr-directive>')($rootScope);
        $rootScope.$digest();
        expect(element.text()).toEqual('init | init');
      });
    });

[petebacondarwin]

Apart from adding a check for the isolated scope, this LGTM

[caitp]

added

[petebacondarwin]

Slightly unrelated to this fix, but should we be bothering to call $observe at all if the attribute doesn't exist?

[caitp]

It might exist later, I think that's the reasoning behind this

[mgol]

The problem is that if you have a one-time binding and the value is sometimes expected to be not defined then in many cases you basically have to write attribute="::null". This is very unintuitive and it's very easy to create lots of excessive bindings.

Changing that might be a breaking change, though. But it might be worth doing in 1.5?

[gkalpak]

Note that it also happens with new scope and bindToController (e.g. scope: true, bindToController: {prop: '@'}), so it might be a good idea to add a test for that too.

Also note that the case where something is returned from the controller behaves differently, so (if one wanted to be thorough) both cases should be tested. In any case, if I had to choose, I would prefer to have the version where something is returned from the controller, as it is the most fragile one:

controller: function () {
  ...
  return <something>;

[caitp]

Honestly trying every permutation of it is going to add like 300 lines, it's the same paths being taken for each one. Let's keep it for now and add more in a follow-up

[petebacondarwin]

I'm going to merge this. Thanks @caitp.
@gkalpak - if you get a moment could you knock up some more tests that would match the cases you mention above? Thanks

[gkalpak]

I don't feel strongly about it, so it lgtm.
_{(But I think the paths are different; i.e. thete are extra paths that call the function again in some cases.)}

[caitp]

Yeah lets do it in a followup. in a meeting atm, if someone else wants to land this