ion-select and ion-range strange behaviour with ChangeDetectionStrategy.OnPush

[RyanMcDonald]

Ionic version: (check one with "x") [ ] 1.x [X] 2.x

I'm submitting a ... (check one with "x") [X] bug report [ ] feature request [ ] support request => Please do not submit support requests here, use one of these channels: https://forum.ionicframework.com/ or http://ionicworldwide.herokuapp.com/

Current behavior: Using this plunker, which uses ChangeDetectionStrategy.OnPush with the ion-select and ion-range components, the bound values don't display on the components. I've put a regular select element beside it to demonstrate how it should work.

If you comment out ChangeDetectionStrategy.OnPush, it works as expected.

Expected behavior: The ion-range and ion-select components should display the values associated with the bound ngModel property.

Steps to reproduce: http://plnkr.co/edit/TjHJJP3AhuAYuDQhyKI0?p=preview

Ionic info: (run ionic info from a terminal/cmd prompt and paste output below): Cordova CLI: 6.3.1 Ionic Framework Version: 2.0.0-rc.3 Ionic CLI Version: 2.1.7 Ionic App Lib Version: 2.1.4 Ionic App Scripts Version: 0.0.46 ios-deploy version: Not installed ios-sim version: Not installed OS: OS X El Capitan Node Version: v6.0.0 Xcode version: Xcode 8.0 Build version 8A218a

[iron9light]

Is it the same issue as #9205 ?

[RyanMcDonald]

It is similar, so the underlying bug might be the same. But that issue is using FormBuilder and is not able to select an option from the drop down at all. My issue is related to inputs being passed into a component where the input is used in the component's ion-select and ion-range values, so the initial load of the component doesn't show the selected value.

[verylongword]

THE FOLLOWING WAS MY MISTAKE: fixed by .cache(1) on the contacts: Observable.

I have a similar issue: ALTHOUGH the following works fine using changeDetection: ChangeDetectionStrategy.OnPush + this.changeDetector.markForCheck() inserted in the definition of ids : Observable<string[]>

` <ion-item *ngFor="let id of ids | async" >{{ id }}

BUT on the same page, another more complex contacts: Observable is displayed (correctly) only when the page is re-loaded. On the first load the following displays nothing:

` <button ion-item *ngFor="let contact of contacts | async" (click)="buttonClickContact(contact)"> CLICK: {{contact.childData.name.getFullName()}}

All the above display correctly when the page is reloaded after exiting the app, Fixed everything by applying .cache(1) to the contacts: Observable

[ghaiat]

Hello, any update on this? it s a pretty annoying issue ...

[ncapito]

Just hit this same issue anyone working on a fix @brandyscarney ?

Here is a short term workaround for this who dont mind a flash. I put this in my component that references the slider.

    constructor(private cf: ChangeDetectorRef) {
    }

    ngAfterViewInit(){
        setTimeout(() =>{
            console.log('Remove once #9374 is fixed');
            this.cf.markForCheck();
        }, 100);

    }

[jgw96]

Hello all! So here are my findings on this so far. First ngModel is not exactly made to be used in a component that is using the onPush change detection. To use ngModel in a component that uses this kind of change detection you will need to use something like @ncapito is using above where he manually checks for change detection. Now the issue here is that with that setup you are going to be triggering change detection every 100ms, which is terrible for performance. Ideally you would write some custom logic to check for dirty values that need to be updated and then trigger change detection based on that. Now because of the way our range component and select component are designed, with normal angular change detection they will work great with ngModel binding but since we do not implement a custom change detection in these components they will not work with onPush change detection. For the time i would recommend not using onPush change detection in your custom component while we discuss all this internally. Thanks!

[jgw96]

Hello @RyanMcDonald so after playing with your plunker some more i figured out atleast part of what was going on with our ion-range component. In your plunker i noticed that the range component was "frozen", IE it would not move at all. After looking i noticed you were using ngModel incorrectly. You had [ngModel] while the correct syntax is [(ngModel)]. I have made a fork of your plunker that shows that the range is moveable now. I am still working on why the change detection is not happening correctls, but hope to have that resolved soon.

[RyanMcDonald]

Thanks @jgw96 for looking into this!

The reason I was using just [ngModel] and not [(ngModel)] was because I only want unidirectional data flow, hence the use of onPush change detection. I don't want the Range component updating the object that I pass in for ngModel. My ion-range component takes an input value, and triggers an output event when its internal value changes, which triggers the update of the data in @ngrx/store, which then passes back the updated data into the component. The reason I had to use [ngModel] is because the component doesn't seem to expose a value property that I can bind to to accomplish this.

[jgw96]

No problem at all! So after a meeting with Misko from the angular team yesterday I now know exactly why this is not working. So if you look here and here you will see that both the range and select components do not use onPush change detection. The main reasoning for this is that the range and select are built to be used with [(ngModel)] which does not work out of the box with onPush change detection because [(ngModel)] is all about two way binding while onPush forces one way bindings. So for full disclosure I am not sure if at this time we want to spend the time to write some custom dirty checking with changeDetectorRef to make range and select work with onPush. We have our milestone meeting tomorrow and ill make sure i bring this one up for consideration for the next milestone. Thanks for using Ionic and thanks for being patient with me on this issue!

[ncapito]

Personally I think this is a big deal. If we were targeting desktop's I wouldn't care to much. But to be targeting mobile devices and not being able to take advantage of the performance benefits of OnPush make the native feel even harder to accomplish.

I understand the ask to fix is huge since most of these input components are built on top of a a larger framework. But what will happen is people will stop being using your inputs & labels and look elsewhere. Is there a compromise here were we could expand ion-item, ion-label, ion-input to allow for someone to "bring their own" input component ? Because I've tried to in the past just build my own input and still use item & label and it just ignores the custom input.

Did any of that make sense?

[comfortme]

is this issue resolved? im still seeing it with 2.1.0

[biesbjerg]

Also an issue for other components like ion-segment.

[jgw96]

Hello all! After some thought, we are going to close this issue. ion-select and ion-range have to use ngModel to get the correct functionality. We maybe could refactor those two components to get the same functionality but with one-way bindings but for multiple reasons that include making the developer experience harder for using these two components, we are not going to do this. Also, any potential performance gains from changing these two components to use onPush, would be very negligible and most likely not even measurable in a full app, even on the slow devices that i use to test with. Thanks everyone for using Ionic!

[marcorinck]

@jgw96

I don't understand the reasoning behind closing this issue. Currently ion-segment fails to render its default value correctly when the developer uses ChangeDetection.onPush ... SILENTLY. This is is a massive bug and should not occur when using standard angular features.

I just had to invest quite some time to finally find the issue and found it just by accident after adding some other controls and seeing that after a click on them ion-segment rendered the default value correctly.

The performance gain of ChangeDetection.onPush may be neglible when you only measure in single ionic components but that is not the point. I use ionic components all over the place in fairly complicated pages where I use a ton of other components. And the overall performance is the sum of all parts, not a single component.

And whats more problematic is that because the component where I use ion-segment needs to be ChangeDetectionStrategy.Default all parent components need to be Default too because when one single component has OnPush angular doesn't check any child components anymore when the data hasn't changed.

I would really like to ask to revisit the decision to not change this behaviour.

[ionitron-bot[bot]]

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.