feat(upgrade): support bindToController with binding definitions

[0x-r4bbit]

Since angular 1.4 we can also pass controller bindings directly to bindToController, making this syntax more convenient

This needs to be implemented I just added a test for it now.

/cc @mhevery

[mhevery]

@PascalPrecht good catch, could you give the implementation a try?

[0x-r4bbit]

@mhevery I can try but surely need some guidance. Oh and earliest after AngularConnect :)

[0x-r4bbit]

@mhevery this is ready for review.

/cc @IgorMinar

[0x-r4bbit]

I realised that my implementation takes either scope bindings or bindToController bindings. However, technically, in an Angular 1 component, you could use both. E.g.

app.directive('foo', function () {
  return {
    bindToController: {
      bar: '@'
    },
    scope: {
      barFoo: '@'
    },
    controller: 'FooController as foo',
    template: '{{foo.bar}} {{barFoo}}'
  };
});

As far as I understand, UpgradeAdapter currently only supports a single destination object where values of bindings are written to (either controller or scope).

How do we want to deal with that? What do we want to support?

[mhevery]

@PascalPrecht I don't think ngUpgrade will ever support / mimic the Angular 1 exactly. (For example we don't and can't support ng1 replace feature) So in this case I think we should just check for this corner case and if this is what the user has requested throw a descriptive error.

[alxhub]

Hi @PascalPrecht,

Please fix Travis errors and then I can merge this PR.

Thanks!

[0x-r4bbit]

@alxhub Alrighty.

So should I not add an error case as @mhevery suggested?

[alxhub]

@PascalPrecht: Please do address @mhevery's comments - it just looks like the "pr_action: merge" label was applied prematurely. That's a signal in our process that a PR is ready to be merged.

No worries!

[0x-r4bbit]

@alxhub this is implemented now. Waiting for travis to be happy.

[alxhub]

Looks like all you need is "gulp check-format".

[0x-r4bbit]

Argh. Forgot it to run when I updated the commit. This should be fine now.

[0x-r4bbit]

Yay, travis is happy. Let's merge it away.

[teropa]

+1 - since module.component in 1.5 always uses bindToController, this is needed for any bindings in such components to work.

[0x-r4bbit]

Oh i thought this has been merged already.

[choeller]

+1 The 1.5 component-API is also not working with upgradeNg1Component currently:

 .component('checked', {
    bindings: {
      check: '='
    },
    template: 'Is checked: {{ctrl.check}}',
    controller: function() {}
  });

in combination with

@Component({
  selector: 'home',
  template: `
    <div>
      <checked [check]="isChecked"></checked>
    </div>
  `,
  directives: [adapter.upgradeNg1Component('checked')]
})
export class Home {
  constructor() {
    this.isChecked = true;
  }  
}

currently leads to:

Can't bind to 'check' since it isn't a known native property ("
      <checked [ERROR ->][check]="isChecked"></checked>

Also the docs on https://angular.io/docs/ts/latest/guide/upgrade.html are saying above is working

[mattdsteele]

Until this gets merged, is there a good way to get upgradeNg1Component to function with bindings in Angular 1 code? Based on this code in the Playground module, you should be able to define the Angular 1 directive's bindings on an isolate scope, but I can't get these to work with the UpgradeAdapter, either.

/cc @choeller @teropa

[0x-r4bbit]

@mattdsteele This should work fine as it's also covered in unit tests. Can you create a plunk demonstrate what doesn't work?

[mattdsteele]

@PascalPrecht Seems like it's working once I upgraded to beta.3 - so no worries. Thanks for merging!

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}