feat(upgrade): Support ng-model in downgraded components #10578

kseamon · 2016-08-08T21:48:45Z

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[x ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
No form of ngModel is supported. Components that require it, such as MdInput are not usable via ngDowngrade.

What is the new behavior?
The AngularJS ng-model can now bind to Components that implement the ControlValueAccessor interface.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

kseamon · 2016-08-09T15:47:01Z

Update: ran gulp lint

naomiblack · 2016-08-27T00:24:40Z

holding this PR until after RC6 settles

kseamon · 2016-10-11T16:40:16Z

Resolved conflicts.

gkalpak · 2016-12-19T14:14:35Z

It might be just me, but I feel this crosses the "too much magic under the hood" boundary. ngUpgrade already performs a significant amount of magic under the hood, so I would rather not add to it, if not necessary.

I do understand that ngModel/ControlValueAccessor are very common interfaces, though, and there should be an ergonomic way to use them in upgraded/downgraded components.

I haven't thought this through, rather thinking out loud:

Would this be possible as an independent directive (i.e. not part of DowngradedNg2Component). It could be even part of ngUpgrade.

petebacondarwin · 2016-12-19T14:22:28Z

I have a similar feeling, although I am not sure we could manage it through an Angular 1 directive since it would not have access to or knowledge of the Angular 2 downgraded component.

In any case I think it would be important to make this a bit more general, perhaps some way of exposing controllers to downgraded components? If that is not already possible?

kseamon · 2016-12-19T14:56:01Z

A generalizable way of exposing controllers would be nice, but you'd then need ng1 directives that are able to work with ng2 controllers. That's probably an ok requirement for user-generated controllers, but ng-model is so fundamental to Angular that excluding it leaves out a huge percentage of otherwise viable components.

It would be good to look at a solution going the other way as well - allowing upgraded ng1 components to interoperate with ng2's NgModel as well.

petebacondarwin · 2016-12-21T08:34:06Z

Actually the more I look at this PR the more I think it is reasonable, given the status of ngModel in the framework.

kseamon · 2017-01-11T16:27:23Z

I'll have time to get this updated next week - would you like me to take a look at this feature for upgraded components as well?

petebacondarwin · 2017-01-11T16:29:54Z

@kseamon - yes that would be great. Also we need it to work for upgrade/static too.

kseamon · 2017-01-19T16:03:09Z

Synced up with master.

kseamon · 2017-01-19T16:05:34Z

Looks like I need to more or less copy these changes into the aot subdirectory as well?

petebacondarwin · 2017-01-19T19:16:41Z

That sounds about right. @gkalpakas is working on a PR to unify the two libraries on some level. But in the meantime there is duplication.

…

On Thu, 19 Jan 2017 at 16:06 Karl Seamon ***@***.***> wrote: Looks like I need to more or less copy these changes into the aot subdirectory as well? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#10578 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA9J2w9ta3J4DVlznUgb6vBT-JiEhpCks5rT4n0gaJpZM4Jfgnw> .

kseamon · 2017-01-19T20:50:55Z

Added in the aot stuff. Unit tests are passing for me but there seems to have been a timeout or tool failure in travis for ios 7. Any way to rerun the tests to see if it is a fluke?

petebacondarwin · 2017-01-19T21:04:08Z

I can rerun them...

kseamon · 2017-01-19T21:17:07Z

One other note: as I'm diving into this again to try to figure out making this work the other way (for upgrading ng1 directives) I noticed that both ng1 and ng2 have hooks for "touched" (which is really blurred - not a touch event). Probably worth adding it, but not going to bother for this PR.

petebacondarwin · 2017-01-19T21:19:12Z

Seems to pass now. I'll review tomorrow.

petebacondarwin

Nothing but minor refactoring that could be done here or in a subsequent PR.

petebacondarwin · 2017-01-20T07:54:16Z

modules/@angular/upgrade/src/aot/downgrade_component_adapter.ts

@@ -47,6 +47,13 @@ export class DowngradeComponentAdapter {
        childInjector, [[this.contentInsertionPoint]], this.element[0]);
    this.changeDetector = this.componentRef.changeDetectorRef;
    this.component = this.componentRef.instance;
+
+    if (this.ngModel && typeof this.component.writeValue === 'function' &&
+        typeof this.component.registerOnChange === 'function') {


I kind of feel like I would like this test to be factored out so that it is clear what we are checking for. E.g. implementsControlValueAccessor(this.component) or similar.

petebacondarwin · 2017-01-20T07:58:01Z

modules/@angular/upgrade/src/downgrade_ng2_adapter.ts

+      this.ngModel.$render = () => { this.component.writeValue(this.ngModel.$viewValue); };
+
+      this.component.registerOnChange(this.ngModel.$setViewValue.bind(this.ngModel));
+    }


Although I said we would need copy duplicate a lot of the code we could actually factor out this whole section of code into a function that is shared between dynamic and static versions. Perhaps put it in https://github.com/angular/angular/blob/master/modules/%40angular/upgrade/src/util.ts?

petebacondarwin · 2017-01-20T07:58:40Z

modules/@angular/upgrade/src/upgrade_adapter.ts

@@ -138,6 +138,9 @@ export class UpgradeAdapter {
   * 2. Even thought the component is instantiated in Angular 1, it will be using Angular 2+
   *    syntax. This has to be done, this way because we must follow Angular 2+ components do not
   *    declare how the attributes should be interpreted.
+   * 3. ng-model is controlled by AngularJS v1 and communicates with the downgraded Ng2 component
+   *    by way of the ControlValueAccessor interface from @angular/forms. Only components that
+   *    implement this interface are elligible.


petebacondarwin · 2017-01-20T08:03:25Z

modules/@angular/upgrade/test/aot/integration/downgrade_component_spec.ts

+             this._onChangeCallback(newValue);
+           }
+         }
+         let ng2Instance: Ng2;


I think this declaration should go above the class, since let is not hoisted.

petebacondarwin · 2017-01-20T08:21:07Z

@kseamon thanks for this. Let me know if you want to make the changes I suggested or just merge this as-is.

kseamon · 2017-01-20T15:19:36Z

These changes are pretty straightforward so I'll take care of them now.

That will also give us some time to discuss where I'm at with upgraded components - I'll start that conversation in slack now.

kseamon · 2017-01-20T18:51:33Z

Suggested changes have been applied.

sharikovvladislav · 2018-03-29T17:03:54Z

@kseamon thank you. +100500 for you karma 🎉 🎉 🙌

This feature solves problem of syncing Angular component state with AngularJS NgFormController.

Does angular have some mechanism for setting validity of NgFormController AngularJS form accordingly to validity of component itself?
I mean: if control in Angular is not valid and it is downgraded component and it is inside of ngForm directive - it does not influence NgFormController in AngularJS.

I can't find anything at the moment.

If it is not, I am sure is not hard to add this. Something like:

if (typeof component.registerSetValidity === 'function') {
  component.registerSetValidity(ngModel.$setTouched.bind(ngModel));
}

What do you think about this?

@kseamon
@alxhub

sharikovvladislav · 2018-03-30T07:51:37Z

I created #23085.

angular-automatic-lock-bot · 2019-09-13T11:51:26Z

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.}

googlebot added the cla: yes label Aug 8, 2016

mhevery added the pr_state: LGTM label Aug 8, 2016

kseamon force-pushed the ng-upgrade-ng-model branch from 56e8d2d to dad6b86 Compare August 9, 2016 15:45

vsavkin added the comp: upgrade label Oct 4, 2016

kseamon force-pushed the ng-upgrade-ng-model branch from dad6b86 to ea9f4f3 Compare October 11, 2016 16:07

vicb assigned petebacondarwin Dec 8, 2016

petebacondarwin mentioned this pull request Dec 23, 2016

NgUpgrade should unify Angular 1 and Angular 2 form validation #9213

Closed

3 tasks

kseamon force-pushed the ng-upgrade-ng-model branch 2 times, most recently from 9e62b98 to 2d0fb3f Compare January 19, 2017 15:43

kseamon force-pushed the ng-upgrade-ng-model branch from 2d0fb3f to cbf595b Compare January 19, 2017 20:04

petebacondarwin approved these changes Jan 20, 2017

View reviewed changes

feat(upgrade): Support ng-model in downgraded components

1e46747

kseamon force-pushed the ng-upgrade-ng-model branch from cbf595b to 1e46747 Compare January 20, 2017 17:59

petebacondarwin added the action: merge The PR is ready for merge by the caretaker label Jan 20, 2017

alxhub merged commit e21e9c5 into angular:master Jan 23, 2017

gkalpak mentioned this pull request Jan 31, 2017

refactor(upgrade): partly unify the implementations of dynamic and static upgrade #14037

Closed

3 tasks

juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

feat(upgrade): Support ng-model in downgraded components (angular#10578)

ba506bb

sharikovvladislav mentioned this pull request Mar 30, 2018

No ability to set validation status of AngularJS form from Angular component (ngModel, Upgrade Provider) #23085

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(upgrade): Support ng-model in downgraded components #10578

feat(upgrade): Support ng-model in downgraded components #10578

kseamon commented Aug 8, 2016

kseamon commented Aug 9, 2016

naomiblack commented Aug 27, 2016

kseamon commented Oct 11, 2016

gkalpak commented Dec 19, 2016

petebacondarwin commented Dec 19, 2016

kseamon commented Dec 19, 2016 •

edited

Loading

petebacondarwin commented Dec 21, 2016

kseamon commented Jan 11, 2017

petebacondarwin commented Jan 11, 2017

kseamon commented Jan 19, 2017

kseamon commented Jan 19, 2017

petebacondarwin commented Jan 19, 2017 via email

kseamon commented Jan 19, 2017

petebacondarwin commented Jan 19, 2017

kseamon commented Jan 19, 2017

petebacondarwin commented Jan 19, 2017

petebacondarwin left a comment

petebacondarwin Jan 20, 2017

petebacondarwin Jan 20, 2017

petebacondarwin Jan 20, 2017

petebacondarwin Jan 20, 2017

petebacondarwin commented Jan 20, 2017

kseamon commented Jan 20, 2017

kseamon commented Jan 20, 2017

sharikovvladislav commented Mar 29, 2018 •

edited

Loading

sharikovvladislav commented Mar 30, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

feat(upgrade): Support ng-model in downgraded components #10578

feat(upgrade): Support ng-model in downgraded components #10578

Conversation

kseamon commented Aug 8, 2016

kseamon commented Aug 9, 2016

naomiblack commented Aug 27, 2016

kseamon commented Oct 11, 2016

gkalpak commented Dec 19, 2016

petebacondarwin commented Dec 19, 2016

kseamon commented Dec 19, 2016 • edited Loading

petebacondarwin commented Dec 21, 2016

kseamon commented Jan 11, 2017

petebacondarwin commented Jan 11, 2017

kseamon commented Jan 19, 2017

kseamon commented Jan 19, 2017

petebacondarwin commented Jan 19, 2017 via email

kseamon commented Jan 19, 2017

petebacondarwin commented Jan 19, 2017

kseamon commented Jan 19, 2017

petebacondarwin commented Jan 19, 2017

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin Jan 20, 2017

Choose a reason for hiding this comment

petebacondarwin Jan 20, 2017

Choose a reason for hiding this comment

petebacondarwin Jan 20, 2017

Choose a reason for hiding this comment

petebacondarwin Jan 20, 2017

Choose a reason for hiding this comment

petebacondarwin commented Jan 20, 2017

kseamon commented Jan 20, 2017

kseamon commented Jan 20, 2017

sharikovvladislav commented Mar 29, 2018 • edited Loading

sharikovvladislav commented Mar 30, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

kseamon commented Dec 19, 2016 •

edited

Loading

sharikovvladislav commented Mar 29, 2018 •

edited

Loading