bug(CdkDragDrop): cdkDropList silently breaks with ngFor over calculated expression

[amdw]

If a cdkDropList is based on a collection that is not a direct member of the component (such as a method on the component that returns an array), the drag-and-drop behaviour does not work.

Reproduction

https://stackblitz.com/github/amdw/drag-drop-repro

There are three drop-lists in this project, all intended to modify the same underlying words array. The first and the last work as expected; the middle one (where the *ngFor is over a calculated expression displayedWords()) does not.

Expected Behavior

I would expect drag-and-drop in the middle drop-list to work in exactly the same as the first and the last one, changing the order of the words in all three lists.

Failing that, I would at least expect to see a compile error here, or a warning on the JavaScript console, or (at a very minimum) clear and prominent warnings in the documentation that drop-lists won't work if they are based on calculated expressions.

Since the programmer has taken explicit responsibility (via a cdkDropListDropped function) for applying the appropriate update to the underlying data structure when list items are dragged and dropped, I don't see any reason in principle why this shouldn't work.

In the last drop-list, this is addressed by converting the list source into a member on the component. However, this requires the programmer to manually update that member whenever the underlying words array changes. This is inconvenient and error-prone, and undermines the automated data flow and change detection which is a key benefit of Angular.

Actual Behavior

The top and bottom drop-lists work as expected (items can be dragged and dropped, and this updates the underlying words array, causing the order of words in all three cards to change). However, dragging and dropping in the middle card has no effect whatsoever. It's as if it wasn't a cdkDropList at all.

There are no compile errors or warnings, or JavaScript console messages warning that something might be wrong. The intended drop-list just silently doesn't work.

I don't see anything in the documentation that states that this type of pattern should not work.

Environment

• Angular: 11.1.0
• CDK/Material: 11.1.0
• Browser(s): Reproduced on Firefox 84.0.2 on Ubuntu and Chrome 87.0.4280.141 on MacOS

Possibly related issues

This issue may be the same as #13340 - I'm not sure, as the wording of that issue is not clear to me. That issue is marked as a feature request, whereas I would regard this as a bug, or at least a surprising undocumented limitation.

[crisbeto]

The problem with returning an array from a method is that you get a completely new instance every time. This means that Angular will try to re-render the list every time change detection runs. Since the drag-drop module is set up to stop the drag sequence when the dragged item is destroyed, it looks like it doesn't work at all. I'm closing the issue, because there isn't much we can do about it and it's something that you should be avoiding in general, because it can lead to poor performance.

[amdw]

Please can this issue be reopened until the limitation can at least be documented? It's not at all obvious and it's super-confusing when you run into it.

Does change detection run during the dragging process, or only when the item is dropped and the cdkDropListDropped handler runs, modifying the underlying data structure?

I would expect the latter - but then I don't see why there would be any performance issues?

[crisbeto]

I'm not sure that this is the best place to document it, considering that it's how the framework behaves. We've had similar issue reports in the past that have been closed without any changes on our end.

In the drag&drop case, change detection runs after dragging has started and when it is done. The performance issues come from the fact that you're returning new arrays all the time, leading to increased memory consumption. This isn't limited to the drag interaction, it'll happen any time the user interacts with something on the page.

[amdw]

I see, thanks for explaining.

When I add a console.log to the displayedWords() function, I see that it fires continuously during drag-and-drop in the middle list. So that does indeed appear to be the root cause of the problem here.

If many users are reporting similar issues to this, then that does seem to point to a documentation gap somewhere.

While I see that it's a general principle of Angular that change detection causes functions called in templates to be re-evaluated, the way this specifically manifests itself in the drag-and-drop library is a bit unusual in my experience. Is there any harm in adding a small statement to the documentation, something like (borrowing from above):

The drag-drop module will stop the drag sequence when the dragged item is destroyed. Angular change detection will run throughout the dragging process. Therefore, drag-and-drop will not work as expected if the dragged items are destroyed during change detection (e.g. because they are the result of a function that will be recalculated during change detection).

If we make this assumption explicit once in the documentation, might that not reduce the number of bug reports like this one?

Once again, thank you very much for your help!

[crisbeto]

The problem is that we'd basically have to add this note everywhere. For what it's worth, this is the first time it has been reported for drag&drop, the previous reports have mostly been from mat-menu where we need to trigger a change detection on mousedown which can prevent click events from firing since the element would be re-rendered before the click sequence is finished.

[amdw]

Fair enough.

I did some research, and it appears there is a no-call-expression ESLint rule that will prevent this problematic pattern, though it's not currently part of the recommended set for templates.

I think I found a better workaround to the problem than creating a new member and manually recalculating it as well. You can use a pipe - although this requires you to create a new array every time it changes:

amdw/drag-drop-repro@fbe1161

For what it's worth, I've been programming in Angular for around two years (admittedly only via side projects) and this is the first time I can recall this pattern in a template causing a correctness issue. Most articles I've read on this subject only talk about this pattern as a threat to performance - for example:

https://medium.com/showpad-engineering/why-you-should-never-use-function-calls-in-angular-template-expressions-e1a50f9c0496

So maybe the drag-and-drop documentation is indeed the wrong place to talk about this, but anecdotally, the correctness implications of function calls in templates appears to be under-appreciated in the Angular community. It's worth thinking about how that might be addressed.

In the meantime, hopefully this issue will help anyone caught out by a similar problem.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}