Skip to content

Conversation

kajetansw
Copy link
Member

@kajetansw kajetansw commented Aug 2, 2020

Problem

When using rxLet directive and binding an observable to it, internally, change detection is scheduled whenever an observable notification (next/error/complete) is sent. There's a possibility (a slight, but...) that a notification is sent and before the actual change detection is triggered, the component is destroyed and stops existing. In other words - change detection is triggered on the non-existing component. That may result with errors or unexpected behaviors.

The other problem this PR solves is triggering change detection on error and complete notifications by using RenderStrategy#detectChanges method instead RenderStrategy#scheduleCD. By using the second one we gain some rendering optimizations specific for the provided rendering strategy.

How to reproduce?

The best way to actually see it is to change libs/template/src/lib/core/render-aware/render-aware_creator.ts, lines 88 and 92 (master branch) and use scheduleCD() method instead of detectChanged().

// libs/template/src/lib/core/render-aware/render-aware_creator.ts

...
tap({
// handle "error" and "complete" notifications for Observable from template
  error: err => {
    console.error(err);
    if (cfg.updateObserver.error) {
      cfg.updateObserver.error(err);
      currentStrategy.detectChanges(); // change to --> currentStrategy.scheduleCD();
    }
  },
  complete: cfg.updateObserver.complete
    ? () => currentStrategy.detectChanges() // change to --> currentStrategy.scheduleCD();
    : undefined
})
...

Tests start to fail on assertions from the detectChanges function from the core Angular. This is because Angular is not happy with triggering change detection on component that doesn't actually exist anymore. Tests are so fast, that they destroy a component under test before the final change detection happens.

Solution

Solution is to define some custom teardown logic for an observable from the view and abort scheduled CD there. This is done with a custom finalizeWithScheduledCD operator.

kajetan.swiatek added 2 commits August 2, 2020 17:02
@BioPhoton BioPhoton added the </> Template @rx-angular/template related label Aug 2, 2020
@BioPhoton
Copy link
Member

I can't make the tests fail. Even if I remove the Aboretcontroller handling completely :D

I will try to enforce the issue first and that use your fix again.

@kajetansw kajetansw mentioned this pull request Aug 7, 2020
@kajetansw kajetansw closed this Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

</> Template @rx-angular/template related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants