ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
51.01k stars 13.51k forks source link

bug: ion-slides swiper instance does not get re-initialized with angular ivy #20356

Closed DavidStrausz closed 4 years ago

DavidStrausz commented 4 years ago

Bug Report

Ionic version:

[x] 5.0.0-rc.2

Current behavior: With angular ivy enabled (9.0.0-rc.12) ion-slides (specifically the swiper instance) does not get re-initialized after destroyed once and is therefore unusable afterwards.

Expected behavior: Swiper instance gets re-initialized, ion-slides keep working.

Steps to reproduce:

Related code:

A sample application via GitHub

Other information: Removing the following line fixes the problem, but I don't think that its a solution to the problem.

https://github.com/ionic-team/ionic/blob/ae4e28969f1d7b3e41f16f4f70c6befdaca6118f/core/src/components/slides/slides.tsx#L153

Related: ngx-swiper-wrapper#226 As ionic is shipping the swiper directly embedded into the framework maybe it needs to be compiled with ngcc once.

Ionic info:

Ionic:

   Ionic CLI                     : 5.4.15 (/usr/local/lib/node_modules/ionic)
   Ionic Framework               : @ionic/angular 5.0.0-rc.2
   @angular-devkit/build-angular : 0.900.0-rc.12
   @angular-devkit/schematics    : 9.0.0-rc.12
   @angular/cli                  : 9.0.0-rc.12
   @ionic/angular-toolkit        : 2.1.2

Cordova:

   Cordova CLI       : 9.0.0 (cordova-lib@9.0.1)
   Cordova Platforms : ios 5.1.1
   Cordova Plugins   : cordova-plugin-ionic-keyboard 2.2.0, cordova-plugin-ionic-webview 4.1.3, (and 4 other plugins)

Utility:

   cordova-res : not installed
   native-run  : not installed

System:

   ios-deploy : 1.9.2
   ios-sim    : 8.0.2
   NodeJS     : v12.4.0 (/usr/local/bin/node)
   npm        : 6.11.3
   OS         : macOS Catalina
   Xcode      : Xcode 11.3.1 Build version 11C504
liamdebeasi commented 4 years ago

Thanks for the issue. I've been digging into this issue, and this behavior appears to be a bug in Angular 9. We are reaching out to the Angular team and will post an update here when we have it.

In the meantime, we recommend sticking with Angular 8.

liamdebeasi commented 4 years ago

Looks like there was a breaking change in Angular 9 that we missed πŸ˜„. I will mark this as a bug and provide an update here when I have more to share.

tetkosimi commented 4 years ago

Hi @liamdebeasi ! ion-slides looks good now, the original issue has been resolved, but there are many new problems with other elements. For example ion-tab-bar is missing, ion-menu isn't working, ion-modal animation looks corrupted. I've tested on iOS.

liamdebeasi commented 4 years ago

Yeah I don't think my PR was the right solution. Going to try another approach.

netsesame2 commented 4 years ago

Got same issue after upgrade to ionic5.0.0. I've two sliders layout in two pages. However the first is fine, and the second, can't work by calling API, such as slideNext, etc.

Addition: if the second slides is opened in a modal dialog, it will be fine.

Waiting for the fix....

sin-bufan commented 4 years ago

so, what is the temp walkaround for this issue now?

sin-bufan commented 4 years ago

@tetkosimi thanks

tetkosimi commented 4 years ago

so, what is the temp walkaround for this issue now?

Use Angular 8, or if you've already upgraded to Angular 9, disable Ivy. https://angular.io/guide/ivy

DavidWiesner commented 4 years ago

Here is a workaround for this issue. Simply postpone the slider initialization with ngIf and the angular lifecycle.

my-component.html

<ion-slides *ngIf="didInit">
   <!-- multiple <ion-slide/> -->
</ion-slides>

my-component.ts

@Component({
    templateUrl: 'my-component.html'
})
export class MyComponent implements AfterViewInit {
    public didInit: boolean = false;

    ngAfterViewInit() {
        this.didInit = true;
    }
}
liamdebeasi commented 4 years ago

Hi everyone,

We are still investigating how to properly fix this. I will post more here when I have another update. In the meantime feel free to use any of the workarounds provided in this thread.

Thanks!

tuif commented 4 years ago

We are facing the problem. Is there any new information on this issue?

bieniasj commented 4 years ago

I can also confirm this problem :) . Hopefully there will be a fix soon πŸ™

webcat12345 commented 4 years ago

Same problem. But I am so happy to see this problem here. :)

Flowrome commented 4 years ago

I think this is related also:

Versions:

Ionic: 5.0.5 Angular: 9.0.6 Capacitor: 1.5.1 Cap Android: 1.5.1 Cap iOS: 1.5.1

On Hybrid (iOS and Android) the slider instance is not callable, so it is not possible to call any method exposed from IonSlides

here some code: Typescript

import { Component, OnInit, ViewChild, AfterViewInit } from '@angular/core';
import { uniqueId } from 'src/app/utils/utilities/ids';
import { Router } from '@angular/router';
import { Store, Select } from '@ngxs/store';
import { ToggleShowWizard } from '../../store/actions/common.action';
import { take } from 'rxjs/operators';
import { IonSlides } from '@ionic/angular';
import { Observable } from 'rxjs';
import { CommonStateInterfaceModel } from '../../store/states/common.state';

interface Slide {
  image: string;
  title: string;
  id: string;
  description?: string;
  button?: {
    label: string;
    cta: (bho?: any) => void;
    icon?: string;
  };
}

@Component({
  selector: 'app-wizard',
  templateUrl: './wizard.page.html',
  styleUrls: ['./wizard.page.scss']
})
export class WizardPage implements OnInit, AfterViewInit {
  public slides: Slide[];
  public index: number = 0;
  public wizard: IonSlides;
  public wizardOptions: any;
  @Select(state => state.common) common$: Observable<CommonStateInterfaceModel>;

  constructor(private router: Router, private store: Store) {
    const t = this;
    this.wizardOptions = {
      on: {
        beforeInit: function() {
          console.log(this.el);
          t.wizard = this.el;
        }
      }
    };
  }

  public async ngOnInit(): Promise<void> {
    const value = await this.common$.pipe(take(1)).toPromise();
    if (!value?.showWizard) {
      this.router.navigate(['/routines'], { replaceUrl: true });
    }
  }

  private async nextSlide(): Promise<void> {
    await this.wizard?.slideTo((await this.wizard?.getActiveIndex()) + 1);
  }

  public ngAfterViewInit(): void {
    this.slides = [
      {
        id: uniqueId('slide_0'),
        image: '../../../assets/images/wizard_welcome.svg',
        title: 'WIZARD.SLIDES.SLIDE_0.TITLE',
        button: {
          label: 'WIZARD.SLIDES.SLIDE_0.BUTTON',
          icon: 'play-outline',
          cta: () => {
            this.nextSlide();
          }
        }
      },
      {
        id: uniqueId('slide_1'),
        image: '../../../assets/images/wizard_routines.svg',
        title: 'WIZARD.SLIDES.SLIDE_1.TITLE',
        description: 'WIZARD.SLIDES.SLIDE_1.DESCRIPTION',
        button: {
          label: 'WIZARD.SLIDES.SLIDE_1.BUTTON',
          icon: 'arrow-forward-outline',
          cta: () => {
            this.nextSlide();
          }
        }
      },
      {
        id: uniqueId('slide_2'),
        image: '../../../assets/images/wizard_add_routine.svg',
        title: 'WIZARD.SLIDES.SLIDE_2.TITLE',
        description: 'WIZARD.SLIDES.SLIDE_2.DESCRIPTION',
        button: {
          label: 'WIZARD.SLIDES.SLIDE_2.BUTTON',
          icon: 'arrow-forward-outline',
          cta: () => {
            this.nextSlide();
          }
        }
      },
      {
        id: uniqueId('slide_3'),
        image: '../../../assets/images/wizard_timers.svg',
        title: 'WIZARD.SLIDES.SLIDE_3.TITLE',
        description: 'WIZARD.SLIDES.SLIDE_3.DESCRIPTION',
        button: {
          label: 'WIZARD.SLIDES.SLIDE_3.BUTTON',
          icon: 'arrow-forward-outline',
          cta: () => {
            this.nextSlide();
          }
        }
      },
      {
        id: uniqueId('slide_4'),
        image: '../../../assets/images/wizard_add_timer.svg',
        title: 'WIZARD.SLIDES.SLIDE_4.TITLE',
        description: 'WIZARD.SLIDES.SLIDE_4.DESCRIPTION',
        button: {
          label: 'WIZARD.SLIDES.SLIDE_4.BUTTON',
          icon: 'arrow-forward-outline',
          cta: () => {
            this.nextSlide();
          }
        }
      },
      {
        id: uniqueId('slide_5'),
        image: '../../../assets/images/wizard_timer_play.svg',
        title: 'WIZARD.SLIDES.SLIDE_5.TITLE',
        description: 'WIZARD.SLIDES.SLIDE_5.DESCRIPTION',
        button: {
          label: 'WIZARD.SLIDES.SLIDE_5.BUTTON',
          cta: () => {
            this.store
              .dispatch(new ToggleShowWizard(false))
              .pipe(take(1))
              .subscribe(() => {
                this.router.navigate(['/routines'], { replaceUrl: true });
              });
          }
        }
      }
    ];
  }
}

HTML:

<ion-content [fullscreen]="true">
  <ion-slides *ngIf="slides?.length > 0" [options]="wizardOptions" id="wizard">
    <ng-container *ngFor="let slide of slides; index as i">
      <ion-slide *ngIf="slide?.id">
        <img [src]="slide.image" />
        <h2>{{slide.title | translate}}</h2>
        <p *ngIf="slide.description">{{slide.description | translate}}</p>
        <ion-button (click)="slides[i].button.cta()" *ngIf="slide.button" fill="clear">
          {{slide.button.label | translate}}
          <ion-icon *ngIf="slide.button.icon" slot="end" [name]="slide.button.icon"></ion-icon>
        </ion-button>
      </ion-slide>
    </ng-container>
  </ion-slides>
</ion-content>

Whenever i call slideTo(...) it doesn't give me anything, this is happening only on iOS/Android on production build (ng build --prod), on dev is working on both, this is probably due to IVY compilation.

Unfurtunately it doesn't give me any error in console (neither on iOS/Android console)

MuhAssar commented 4 years ago

after I had spent too much time tracking down this issue I assembled a working proof,

then I came here to submit the bug with my proof and voila I find this bug report for the exact same bug :)

any updates @liamdebeasi ?

edit: I tried disabling ivy but that introduced more bugs, I also tried the *ngIf="didInit" workaround mentioned earlier but unfortunatley didn't solve the problem.

edit2: I settled with disabling ivy and fixing resulting bugs until a proper fix is released.

nikosdouvlis commented 4 years ago

Hello everyone. I can confirm the same behaviour on the latest ionic and angular versions.

hanibhat commented 4 years ago

Here is a workaround for this issue. Simply postpone the slider initialization with ngIf and the angular lifecycle.

my-component.html

<ion-slides *ngIf="didInit">
   <!-- multiple <ion-slide/> -->
</ion-slides>

my-component.ts

@Component({
    templateUrl: 'my-component.html'
})
export class MyComponent implements AfterViewInit {
    public didInit: boolean = false;

    ngAfterViewInit() {
        this.didInit = true;
    }
}

I had to a timer and then it worked. Thanks!

Flowrome commented 4 years ago

Here is a workaround for this issue. Simply postpone the slider initialization with ngIf and the angular lifecycle. my-component.html

<ion-slides *ngIf="didInit">
   <!-- multiple <ion-slide/> -->
</ion-slides>

my-component.ts

@Component({
    templateUrl: 'my-component.html'
})
export class MyComponent implements AfterViewInit {
    public didInit: boolean = false;

    ngAfterViewInit() {
        this.didInit = true;
    }
}

I had to a timer and then it worked. Thanks!

Hi @hanibhat as you can see from my code i already implement a solution like this one, i initialize the slides variable in the ngAfterViewInit function, and the ion-slides render only if slides.length > 0, so it doesn't work in my case sorry about that.

hanibhat commented 4 years ago

@Flowrome I had no idea how to fix it until I read your work around. Thanks a lot!

liamdebeasi commented 4 years ago

Can everyone try the following dev build and let me know if it resolves the issue? I tested in the repo provided in https://github.com/ionic-team/ionic/issues/20356#issue-558204094, and everything seems to work.

npm i @ionic/angular@5.1.0-dev.202003271919.8a161f0

Thanks!

DavidWiesner commented 4 years ago

@liamdebeasi did you find out why the slider is initialized (or connected) multiple times?

MuhAssar commented 4 years ago

@liamdebeasi nice work I think this fixed the bug, but I noticed the following:

using this options: public options = { on: { beforeInit() { console.log('beforeInit'); }, beforeDestroy(){ console.log('beforeDestroy'); } } }

if ivy is enabled the beforeInit event is called twice, if ivy is disabled it is called once only as it should be.

Flowrome commented 4 years ago

Can everyone try the following dev build and let me know if it resolves the issue? I tested in the repo provided in #20356 (comment), and everything seems to work.

npm i @ionic/angular@5.1.0-dev.202003271919.8a161f0

Thanks!

i'll try this evening if everything works when we can expect the prod release? i'd like to not have dev branches on production environment 😁

liamdebeasi commented 4 years ago

@liamdebeasi did you find out why the slider is initialized (or connected) multiple times?

To summarize, Angular Ivy creates new pages as siblings of their router outlets, but Ionic Framework requires them to be children of the router outlets.

What Angular Ivy wants:

<ion-router-outlet></ion-router-outlet>
<my-page-with-slides></my-page-with-slides>

What Ionic Framework wants:

<ion-router-outlet>
  <my-page-with-slides></my-page-with-slides>
</ion-router-outlet>

To work around this, we call ionRouterOutlet.appendChild(myPageWithSlides) which is going to "move" the page from being a sibling to being a child of the router outlet.

When Web Components are attached to the DOM, the connectedCallback method is called, and when Web Components are removed from the DOM, the disconnectedCallback method is called. So moving a component is going to cause connectedCallback to be called, followed by disconnectedCallback, followed by another connectedCallback.

The problem with ion-slides is that the disconnectedCallback method was asynchronous, meaning it was possible for the Swiper instance to be destroyed after the second connectedCallback, resulting in ion-slides being left in a destroyed state.

i'll try this evening if everything works when we can expect the prod release? i'd like to not have dev branches on production environment

We do not typically comment on exact timing of releases, but we are in a regular release cycle with new releases every few weeks.

Flowrome commented 4 years ago

@liamdebeasi did you find out why the slider is initialized (or connected) multiple times?

To summarize, Angular Ivy creates new pages as siblings of their router outlets, but Ionic Framework requires them to be children of the router outlets.

What Angular Ivy wants:

<ion-router-outlet></ion-router-outlet>
<my-page-with-slides></my-page-with-slides>

What Ionic Framework wants:

<ion-router-outlet>
  <my-page-with-slides></my-page-with-slides>
</ion-router-outlet>

To work around this, we call ionRouterOutlet.appendChild(myPageWithSlides) which is going to "move" the page from being a sibling to being a child of the router outlet.

When Web Components are attached to the DOM, the connectedCallback method is called, and when Web Components are removed from the DOM, the disconnectedCallback method is called. So moving a component is going to cause connectedCallback to be called, followed by disconnectedCallback, followed by another connectedCallback.

The problem with ion-slides is that the disconnectedCallback method was asynchronous, meaning it was possible for the Swiper instance to be destroyed after the second connectedCallback, resulting in ion-slides being left in a destroyed state.

i'll try this evening if everything works when we can expect the prod release? i'd like to not have dev branches on production environment

We do not typically comment on exact timing of releases, but we are in a regular release cycle with new releases every few weeks.

still good enough for me if it solve my use case, thank you 😁

liamdebeasi commented 4 years ago

@abuassar Can you try this dev build and let me know if it fixes the duplicate events?

npm i @ionic/angular@5.1.0-dev.202003272115.6f84a4f

MuhAssar commented 4 years ago

@liamdebeasi very nice, I tried it and there are no more duplicated init yay!

Flowrome commented 4 years ago

Can everyone try the following dev build and let me know if it resolves the issue? I tested in the repo provided in #20356 (comment), and everything seems to work.

npm i @ionic/angular@5.1.0-dev.202003271919.8a161f0

Thanks!

Hi @liamdebeasi i've installed the package that you've gave us, it's working fine for my case too, also on production build.

I don't see many breaking changes for my Ionic usage, so i'll publish an update with this version waiting for the next production one.

Thank you for your work

eduboxgithub commented 4 years ago

I was desperate trying to figure out what was going on with an implementation. I need to navigate to a slide index after clicking on an ion-segment.

Sometimes, (90% of the time) the implementation does not work. The slideTo method did not resolve and nothing happens. I tested the version (npm i @ionic/angular@5.1.0-dev.202003271919.8a161f0) and the problem seems to have been solved.

DavidStrausz commented 4 years ago

@liamdebeasi The dev-build fixes the issue for me too, thank you!

liamdebeasi commented 4 years ago

Thanks for the issue. This has been resolved via https://github.com/ionic-team/ionic/pull/20899 and will be available in an upcoming release of Ionic Framework.

DavidWiesner commented 4 years ago

To work around this, we call ionRouterOutlet.appendChild(myPageWithSlides) which is going to "move" the page from being a sibling to being a child of the router outlet.

@liamdebeasi will this be a performance issue? Because swiper than is always initialized twice in angular.

liamdebeasi commented 4 years ago

No, the PR I added prevents this performance issue from happening.

hannata commented 4 years ago

@liamdebeasi thanks it works now for me, but when i add the dir="rtl" the direction does not work

AmandaAdams commented 4 years ago

Any update on when this will be released? This is holding our app from moving forward as we're not comfortable finalizing and wrapping up testing with a dev build. Please advise.

liamdebeasi commented 4 years ago

@hannata Please open a new issue.

@AmandaAdams We hope to have this update out soon.

webcat12345 commented 4 years ago

Waiting on this, because of this slider we disabled ivy renderer for entire project.

liamdebeasi commented 4 years ago

Hi everyone,

This issue is resolved, and we hope to have an update out soon. For any additional bugs please open a new issue. Thanks!