Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#442 breaks with onpush issues #453

Closed
chivesrs opened this issue May 16, 2023 · 11 comments
Closed

#442 breaks with onpush issues #453

chivesrs opened this issue May 16, 2023 · 11 comments

Comments

@chivesrs
Copy link
Contributor

(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 044a294#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

@chivesrs chivesrs added the bug label May 16, 2023
@santoshyadavdev
Copy link
Member

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
Copy link
Collaborator

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

@chivesrs
Copy link
Contributor Author

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
Copy link
Member

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

@chivesrs
Copy link
Contributor Author

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
Copy link
Member

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

@santoshyadavdev
Copy link
Member

Released with 7.2.0

@chivesrs
Copy link
Contributor Author

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

@chivesrs
Copy link
Contributor Author

@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).

Copy link

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 github-actions bot added the Stale label May 24, 2024
Copy link

Closing this issue due to no activity for 6 months.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants