Revert "fix: check length only on unwrapped data (#442)"

[chivesrs]

This reverts commit 044a29499caff4bf2a9606711b21e8da9fefc570 and a3b7f50844a3e3133b59f00b65d65d5fb6bb736f.

Fixes #453

@santoshyadavdev

[nx-cloud[bot]]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 634001f1708b7e6775e7577660af61264e7ab511. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 1 target

- [`nx run-many --target=build --all`](https://cloud.nx.app/runs/4GAKNuC2PZ)

---

Sent with 💌 from NxCloud.

[chivesrs]

@santoshyadavdev just checking on this

[santoshyadavdev]

@santoshyadavdev just checking on this

I was travelling let me take a look

[santoshyadavdev]

chivesrs

BTW our sample app seems to use OnPush strategy and works fine.

[chivesrs]

@santoshyadavdev what do you reckon we should do here? Should I rebase and do my best to get the commit reverted and fix merge conflicts? Something else?

[santoshyadavdev]

Hey buddy, Sorry I just came yesterday from a conference, I will merge this one, can you resolve the conflict please.

[chivesrs]

I've resolved the merge conflict but unfortunately had to revert #458, I tried to preserve the PR as best as I could but I think it depends too much on the changes from #442. I manually brought forward the changes but they didn't work so I think it's safer to have @luca-peruzzo do their fix after this one goes through.

@santoshyadavdev lmk your thoughts

[luca-peruzzo]

Hi @chivesrs, did you try my changes with ChangeDetectionStrategy.OnPush? I think there is no need for _unwrappeddata. Do you have a sample app where I can try to solve the OnPush Issue (if it persists)?

[chivesrs]

@luca-peruzzo the code I have is internal to Google so it's not super easy for me to post it here. I tried your changes out yes on both my app and the sample app in this repo. It worked on the sample app but not on mine. We basically just have one data array and the issue I have is the entire carousel doesn't render until I use the tab key to get into where the next/previous buttons would be, and that causes a change detection cycle which finally renders everything.

I'm open to any suggestions you have but #442 was the one that broke us, and I couldn't port forward #458 easily, as when I did port it forward, the fix didn't work in the sample app.

[chivesrs]

@santoshyadavdev lmk what you think

@luca-peruzzo it may be best you rebase your fix on top of this PR?

[luca-peruzzo]

@santoshyadavdev lmk what you think

@luca-peruzzo it may be best you rebase your fix on top of this PR?

@chivesrs For my pull request, I have taken inspiration from the source code of Angular ngFor, so my main change was to change dataSource type from Observable to NgIterable. Do you use in your code an Observable as input? Because I don't think it is a best practice to subscribe in the plugin, maybe it's better to pass directly subscribed data or use the async pipe... The changes in your code regard the method _observeRenderChanges() that I removed in my pull request, so I think that my changes and yours are incompatible. I see you have problems sharing your code because is internal to Google, but could you provide a similar implementation? if yes I would check what's wrong with my PR and I would fix it. lmk

[santoshyadavdev]

I am with @luca-peruzzo on this one, Its hard to decide if we should rollback without a proper reproduction of the issue,

@chivesrs If we can get a small reproduction, it can help to fix this. We don't want the entire code, just a small reproduction.

[chivesrs]

We just have a raw array of objects, we don't pass in an observable. I will see what I can do about getting you a repro.

[Yberion]

Hello, I've also noticed some problem on my side, what's the plan regarding this MR ? ^^

[santoshyadavdev]

Hey @Yberion we are actually looking for a reproduction to see the actual issue and fix.

Can you create one if possible

That would be a great help.

[chivesrs]

Sorry just been super caught up with non work stuff, getting you a repro is still on my todo list.

[santoshyadavdev]

Sorry just been super caught up with non work stuff, getting you a repro is still on my todo list.

Thank you 😊

[luca-peruzzo]

@chivesrs @santoshyadavdev I finally find a way to reproduce the issue. Working in ngu-carousel-example folder, firstly I had to change all components change detection strategy to OnPush and after I created a wrapper component that accepts input datasource and config for ngu-carousel. html:

<div class="wrapped-carosuel" *ngIf="carouselTileItems.length">
  <h2>Wrapped Carousel</h2>
  <ngu-carousel #myCarousel [inputs]="config" [dataSource]="carouselTileItems">
    <button NguCarouselPrev class="leftRs" [style.opacity]="myCarousel.isFirst ? 0.5 : 1">
      &lt;
    </button>

    <ngu-tile *nguCarouselDef="let item; index as i; let ani = animate" [@slider]="ani">
      <ng-container *ngIf="i % 2 === 0">
        <ng-container
          *ngTemplateOutlet="card; context: { $implicit: item, index: i }"
        ></ng-container>
      </ng-container>
      <ng-container *ngIf="i % 2 !== 0">
        <div class="tile" [style.background]="'url(' + item + ')'">
          <h1>Odd: {{ i + 1 }}</h1>
        </div>
      </ng-container>
    </ngu-tile>

    <button NguCarouselNext class="rightRs" [style.opacity]="myCarousel.isLast ? 0.5 : 1">
      &gt;
    </button>

    <ul class="myPoint" NguCarouselPoint>
      <li
        *ngFor="let i of myCarousel.pointNumbers"
        [class.active]="i === myCarousel.activePoint"
        (click)="myCarousel.moveTo(i)"
      ></li>
    </ul>
  </ngu-carousel>
</div>

<ng-template #card let-item let-i="index">
  <div class="tile" [style.background]="'url(' + item + ')'">
    <h1>Even: {{ i + 1 }}</h1>
  </div>
</ng-template>

ts:

import {
  AfterViewInit,
  ChangeDetectionStrategy,
  ChangeDetectorRef,
  Component,
  Input,
  OnChanges,
  SimpleChanges
} from '@angular/core';
import { NguCarouselConfig } from '@ngu/carousel';
import { slider } from '../../slide-animation';

@Component({
  selector: 'app-wrapped-carousel',
  templateUrl: 'wrapped-carousel.component.html',
  styleUrls: ['./wrapped-carousel.component.scss'],
  animations: [slider],
  changeDetection: ChangeDetectionStrategy.OnPush
})
export class WrappedCarouselComponent implements OnChanges, AfterViewInit {
  @Input() carouselTileItems: any[];
  public config: NguCarouselConfig = {
    grid: { xs: 1, sm: 2, md: 3, lg: 4, xl: 4, all: 0 },
    gridBreakpoints: { sm: 480, md: 768, lg: 1024, xl: 1280 },
    speed: 750,
    point: {
      visible: true
    },
    velocity: 0.1,
    touch: true,
    easing: 'cubic-bezier(0, 0, 0.2, 1)'
  };
  @Input() grid: { xs?: number; sm?: number; md?: number; lg?: number; xl?: number; all?: number } =
    { xs: 1, sm: 2, md: 3, lg: 4, xl: 4, all: 0 };
  constructor(private cd: ChangeDetectorRef) {}

  ngOnChanges(changes: SimpleChanges) {
    if (!!changes?.grid) {
      this.config.grid = changes?.grid?.currentValue || {
        xs: 1,
        sm: 2,
        md: 3,
        lg: 4,
        xl: 4,
        all: 0
      };
    }
  }
  ngAfterViewInit(): void {
    this.cd.detectChanges();
  }
}

This particular configuration triggers ngDoCheck only once but _defDirectives is undefined, so we never call renderNodeChanges()

[santoshyadavdev]

Thank you @luca-peruzzo I will check. This is really useful.

[chivesrs]

Thank you so much for reproducing. Yes, our components are structured similarly, everything is OnPush and we have the carousel wrapped in a few parent components, and they get their data via Angular HttpClient.

Your findings seem similar to what I had stated in #453 right?

[luca-peruzzo]

@santoshyadavdev I found a way to solve the problem. Since ngDoCheck fires before afterContentInit and never updates, I modified ngDoCheck and ngAfterContentInit to call the same method. I tested the behaviour with OnPush and without. I also checked the server-side rendering with Universal and everything seems to be OK. If you want I can do a PR. Let me know.

[santoshyadavdev]

@luca-peruzzo that would be great I can merge and get it released today.

[luca-peruzzo]

@luca-peruzzo that would be great I can merge and get it released today.

Ok, do you want also the reproduction environment with server-side rendering in the PR?

[santoshyadavdev]

If we can that's great, or if you want to add it in another PR that will work too.

Whatever works for you. I am happy to accept the PR.

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity for 6 months. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions[bot]]

Closing this PR due to no activity for 6 months.