DEV mode change detection - Show warning if object reference changes in subsequent check

[ibedard16]

🚀 feature request

Relevant Package

This feature request is for @angular/core

Description

When Angular does change detection, it will re-render the view whenever a property binding changes. There is a slight issue with what Angular considers a "change". If an object reference changes to an object identical to the first one, Angular will update the view with this new object, even if the view ends up being identical to the previous one. This situation is easy for new developers to fall into, as it very easy to write a function like:

divClasses() {
  return {
    red: this.isRed
  };
}

and then bind it to an [ngClass] directive, which will remove and re-apply the red class every change detection call, because divClasses() returns a new object every time it is run.

<div [ngClass]="divClasses()"></div><!-- This div will be updated every time change detection is ran. -->

This causes two big problems, the first being inefficient view rendering. Change detection only needs to update the view whenever the component's isRed value changes, but it ends up updating the view every single time change detection is run. This would slow down the page and negatively impact the end user's experience.

The less obvious problem is the possibility of change detection recursion. If the application has a MutationObserver somewhere, watching for DOM changes, the above function will cause it to run every time change detection is ran. If this MutationObserver also triggers a change detection check, the page will hang as the divClasses() function updates the view, triggering the MutationObserver, which triggers change detection again. This might not even directly be the developer's fault, as the MutationObserver could be from a 3rd party library.

I recently spent a day and a half attempting to debug an application hanging in this way. Funnily enough, the problem was made worse by a 3rd party library which modified console.log to run change detection - which caused recursion to happen again. Ultimately, I suspect this scenario might be the cause of issues like valor-software/ngx-bootstrap#3047, #20450, or #24614.

Describe the solution you'd like

To be clear, my proposed "fix" does NOT involve modifying Angular's change detection to deeply compare object references. Discussion on this bug: #24614 was ultimately tabled because the proposed fix involved deep equality checks, which would be inefficient and very time consuming to check larger objects.

Ultimately, the issue here seems to be with functions like my example divClasses(), which inadvertently abuse change detection to unnecessarily update the view. If there was some way to alert the developer that a function they've written was possibly faulty, that would probably be the way to go.

Currently, Angular displays warnings for change detection when the application is running in dev mode for a similar scenario. If change detection itself causes a property/function to change its output, Angular currently displays a warning about what happened. Could it be possible to display a similar warning for whenever a function returns a different object reference on subsequent change detection calls? A reference check would not be very expensive, and this may end up helping the application developer optimize their program.

[un33k]

@ibedard16 I think you misunderstood the reason behind #24614. The ticket did not call for deep compares. It simply stated that use of a getter that returns an object can have irrecoverable side effect in production and it would be great if we have a warning in dev mode or perhaps at lint time.

[angular-robot]

Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.

Find more details about Angular's feature request process in our documentation.

[ibedard16]

@un33k

From what I understood reading #24614, your bug report describes the issue I'm attempting to address with this feature request. I was wrong in saying your bug report "proposed [a] fix" - your report was only describing the problem and was only looking for a solution.

Reading through your bug report thread, the report was closed as soon as the discussion turned towards "deep change detection". The closer then assumed that addressing your bug would require deep change detection (which it would not). Deep change detection is obviously be a bad idea - and I don't think I need to explain why here.

You did suggest displaying a warning/error message if change detection spent too long executing a single function (#24614). While I agree that the scenario deserves a warning, I'm not sure that's the condition that should be used to display a warning message.

I opened this feature request because I thought a warning during dev mode change detection would be a reasonable fix. There was a brief discussion about whether or not the "Expression has changed after it was checked" warning would be thrown in this scenario, since the object references are changing. Unfortunately, that warning does not get thrown when an object reference changes - so it doesn't appear in this scenario.

Sorry about the confusion, and this late reply.

[angular-robot]

Thank you for submitting your feature request! Looks like during the polling process it didn't collect a sufficient number of votes to move to the next stage.

We want to keep Angular rich and ergonomic and at the same time be mindful about its scope and learning journey. If you think your request could live outside Angular's scope, we'd encourage you to collaborate with the community on publishing it as an open source package.

You can find more details about the feature request process in our documentation.

[pkozlowski-opensource]

Related #25518