uiuniversal / ngu-carousel

Angular Universal carousel
https://ngu-carousel.netlify.app
MIT License
326 stars 105 forks source link

#442 breaks with onpush issues #453

Closed chivesrs closed 1 month ago

chivesrs commented 1 year ago

(this is coming from code inside Google's internal code base, we use ngu-carousel for some of our Angular applications)

Describe the bug

442 breaks initial carousel loading in an OnPush enabled application, where the carousel items do not render until you tab into the carousel via keyboard (as keyboard events trigger a change detection event)

The commit right before it fc0f98580590355d8a078b2f71c7a902e8d72730 works just fine without any issues. The issue I believe is https://github.com/uiuniversal/ngu-carousel/commit/044a29499caff4bf2a9606711b21e8da9fefc570#diff-80e3ec9310b23dec2353dad30b6d6e0d26bf487117733a72fdb8c40c38a95655L190, where there is a time of check vs time of use issue, basically this._unwrappedData is always undefined there, and so the initial ngDoCheck() does nothing, causing this issue.

I'll send out a PR to revert this commit, unfortunately it will break some users that rely on newer functionality but I think it's better so that older users don't have a bug on a simple upgrade.

@arturovt @santoshyadavdev

santoshyadavdev commented 1 year ago

Thank you for Reporting it, as of now I think it's safe to rollback. You can open a PR and we can get it in later with backwards compatibility.

arturovt commented 1 year ago

I think we can just set unwrappedData if Array.isArray(dataSource) and not an Observable.

chivesrs commented 1 year ago

Unfortunately I'm not super familiar with the code base, I'll go with a rollback, and you can add me on a PR that contains a roll forward along with a patch, and I can patch it in and see if it works in our code base.

santoshyadavdev commented 11 months ago

@chivesrs I have merged the PR from @luca-peruzzo can you try the code from main, I will release a new version soon.

chivesrs commented 11 months ago

Hello @santoshyadavdev I have patched in the code from main into our repository. On an initial glance, everything seems to be working fine - the bug I had reported does not seem to be reproducible with the fix @luca-peruzzo made. I will go through the steps to get the change submitted internally and let you know the results. Expect a week or two turnaround time.

santoshyadavdev commented 11 months ago

Sounds good meanwhile I will get it released, so we can merge our standalone component changes

santoshyadavdev commented 11 months ago

Released with 7.2.0

chivesrs commented 11 months ago

I'm currently having some issues where our unit tests are broken that I'm trying to figure out.

chivesrs commented 11 months ago

@luca-peruzzo Can you check what happens if the carousel loses items? The test I have has 2 items in the carousel, user action deletes 1, and a new array is created and passed into the carousel. The carousel then seems to still have 2 items instead of the desired 1 (the deleted one gets shifted to the end).

github-actions[bot] commented 1 month ago

This issue 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] commented 1 month ago

Closing this issue due to no activity for 6 months.