Skip to content

Conversation

hoebbelsB
Copy link
Member

Description

The current implementation returns [] when no changes were detected from the IterableDiffer. This causes the switchMap to stop emitting values which in turn results in no renderCallback called.

// list-template-manager.ts 119
switchMap(([{ changes, items }, strategy]) => {
          if (!changes) {
            return [];
          }

When u put an empty array [] into the rxFor directive (aka listManager), the renderCallback won't emit properly. Imho a new value which results in no change should trigger renderCallback since the change got processed.

Fix

// list-template-manager.ts 119
switchMap(([{ changes, items }, strategy]) => {
          if (!changes) {
            return of([]); // of should be fine even though it completes since this gets ignored currently? Do we have to keep future rxComplete, rxError templates in mind and use NEVER.pipe(mapTo([])) instead??
          }

@hoebbelsB hoebbelsB requested a review from BioPhoton March 8, 2021 22:28
@github-actions github-actions bot added the 🛠️ CDK CDK related label Mar 8, 2021
@hoebbelsB hoebbelsB added the 🐞 Bug Bug: Something isn't working label Mar 8, 2021
@@ -118,7 +118,7 @@ export function createListTemplateManager<
// Cancel old renders
switchMap(([{ changes, items }, strategy]) => {
if (!changes) {
return [];
return of([]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@BioPhoton BioPhoton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. THX!!

@BioPhoton BioPhoton merged commit 220be51 into master Mar 14, 2021
@BioPhoton BioPhoton deleted the fix-list-manager-no-values branch March 14, 2021 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 Bug Bug: Something isn't working 🛠️ CDK CDK related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants