X Tutup
Skip to content

feat(upgrade): support bindToController with binding definitions#4784

Closed
0x-r4bbit wants to merge 1 commit intoangular:masterfrom
0x-r4bbit:upgrade-bindtocontroller
Closed

feat(upgrade): support bindToController with binding definitions#4784
0x-r4bbit wants to merge 1 commit intoangular:masterfrom
0x-r4bbit:upgrade-bindtocontroller

Conversation

@0x-r4bbit
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor

mhevery commented Oct 16, 2015

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

@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

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

@mhevery mhevery self-assigned this Oct 29, 2015
@naomiblack naomiblack modified the milestone: beta-00 Nov 2, 2015
@0x-r4bbit 0x-r4bbit force-pushed the upgrade-bindtocontroller branch from b71c324 to 0e34b7a Compare November 13, 2015 17:56
@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

@mhevery this is ready for review.

/cc @IgorMinar

@0x-r4bbit 0x-r4bbit force-pushed the upgrade-bindtocontroller branch from 0e34b7a to 4b18ed5 Compare November 13, 2015 18:02
@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

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 mhevery added pr_state: LGTM action: merge The PR is ready for merge by the caretaker labels Nov 18, 2015
@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Nov 18, 2015

@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 alxhub assigned 0x-r4bbit and unassigned mhevery Nov 18, 2015
@alxhub
Copy link
Copy Markdown
Member

alxhub commented Nov 18, 2015

Hi @PascalPrecht,

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

Thanks!

@alxhub alxhub added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Nov 18, 2015
@0x-r4bbit 0x-r4bbit force-pushed the upgrade-bindtocontroller branch from 4b18ed5 to b7c59b2 Compare November 18, 2015 19:31
@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

@alxhub Alrighty.

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

@alxhub
Copy link
Copy Markdown
Member

alxhub commented Nov 18, 2015

@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 0x-r4bbit force-pushed the upgrade-bindtocontroller branch from b7c59b2 to ce82f57 Compare November 18, 2015 20:42
@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

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

@alxhub
Copy link
Copy Markdown
Member

alxhub commented Nov 18, 2015

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

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

Closes angular#4784
@0x-r4bbit 0x-r4bbit force-pushed the upgrade-bindtocontroller branch from ce82f57 to 410b1df Compare November 18, 2015 21:45
@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

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

@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

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

@IgorMinar IgorMinar modified the milestones: beta.0, beta.1 Dec 15, 2015
@teropa
Copy link
Copy Markdown
Contributor

teropa commented Dec 21, 2015

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

@0x-r4bbit
Copy link
Copy Markdown
Contributor Author

Oh i thought this has been merged already.

@choeller
Copy link
Copy Markdown
Contributor

+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

@mhevery mhevery modified the milestones: beta.01, beta.02 Jan 25, 2016
@mattdsteele
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Contributor Author

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

@naomiblack naomiblack added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 1, 2016
alexeagle pushed a commit that referenced this pull request Feb 2, 2016
Since angular 1.4 we can also pass controller bindings directly to bindToController, making this syntax more convenient

Closes #4784
@alexeagle alexeagle closed this in 99e6500 Feb 2, 2016
@mattdsteele
Copy link
Copy Markdown

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

@angular-automatic-lock-bot
Copy link
Copy Markdown

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

X Tutup