-
Notifications
You must be signed in to change notification settings - Fork 104
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
Revert "fix: check length only on unwrapped data (#442)" #455
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 634001f. 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 targetSent with 💌 from NxCloud. |
@santoshyadavdev just checking on this |
I was travelling let me take a look |
BTW our sample app seems to use OnPush strategy and works fine. |
@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? |
Hey buddy, Sorry I just came yesterday from a conference, I will merge this one, can you resolve the conflict please. |
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 |
Hi @chivesrs, did you try my changes with ChangeDetectionStrategy.OnPush? I think there is no need for _unwrappeddata. |
@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. |
@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. |
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. |
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. |
Hello, I've also noticed some problem on my side, what's the plan regarding this MR ? ^^ |
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. |
Sorry just been super caught up with non work stuff, getting you a repro is still on my todo list. |
Thank you 😊 |
@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.
ts:
This particular configuration triggers ngDoCheck only once but _defDirectives is undefined, so we never call renderNodeChanges() |
Thank you @luca-peruzzo I will check. This is really useful. |
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? |
@santoshyadavdev I found a way to solve the problem. |
@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? |
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. |
close uiuniversal#453 resolve uiuniversal#455
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. |
Closing this PR due to no activity for 6 months. |
This reverts commit 044a294 and a3b7f50.
Fixes #453
@santoshyadavdev