react-dnd can call into an unmounted component causing Uncaught Error : Could not find a valid targetId

[mx2323]

Describe the bug
it is possible for react-dnd to call a collect function that causes a component to unmount, then make another collect() call on the unmounted component, causing an Uncaught Error "Could not find a valid targetId'.

Redux on it's dispatch() of backend Drag And Drop events makes a copy of all listeners to subscribed events at the beginning and always calls them regardless of what redux subscriptions have been unsubscribed while notifying subscribers (see https://stackoverflow.com/questions/43356080/redux-unsubscribe-within-componentwillunmount-still-calls-subscribe-callback). This means that if something like an 'EndDrag' html5 drag event triggers a notification in handleChange() in react-dnd/src/decorateHandler.tsx:handleChange(), and the first collect() call causes an unmount of a component, the subsequent unmounted component may have its collect() call triggered in a subsequent notification of the same event.

Thus, the invariant 'Expected to find a valid target' is triggered because the component validly has been removed from the registry of valid target Id's, but the notification is calling into an unmounted component.

I discovered this issue while attempting to integrate react-sortable-tree, which uses react-dnd into my app. You can see an example of the issue and a recreatable test case here : frontend-collective/react-sortable-tree#490. A drag and drop that gets cancelled by releasing a dragSource outside of the dragTarget reproduces this issue everytime.

The uncaught error and stack trace is pasted below, The stack trace shows that the connect function is calling into monitor.canDrop() which fails on unmounted components.
browser.js:38 Uncaught Invariant Violation: Expected to find a valid target.
at invariant (https://xzoq6xprlz.codesandbox.io/node_modules/invariant/browser.js:38:15)
at DragDropMonitorImpl.canDropOnTarget (https://xzoq6xprlz.codesandbox.io/node_modules/dnd-core/lib/cjs/DragDropMonitorImpl.js:67:9)
at DropTargetMonitorImpl.canDrop (https://xzoq6xprlz.codesandbox.io/node_modules/react-dnd/lib/cjs/DropTargetMonitorImpl.js:24:41)
at nodeDropTargetPropInjection (https://xzoq6xprlz.codesandbox.io/node_modules/react-sortable-tree/dist/index.cjs.js:2367:28)
at DragDropContainer.getCurrentState (https://xzoq6xprlz.codesandbox.io/node_modules/react-dnd/lib/cjs/decorateHandler.js:116:29)
at DragDropContainer._this.handleChange (https://xzoq6xprlz.codesandbox.io/node_modules/react-dnd/lib/cjs/decorateHandler.js:45:39)
at handleChange (https://xzoq6xprlz.codesandbox.io/node_modules/dnd-core/lib/cjs/DragDropMonitorImpl.js:27:21)
at dispatch (https://xzoq6xprlz.codesandbox.io/node_modules/redux/lib/redux.js:220:7)
at Object.eval [as endDrag] (https://xzoq6xprlz.codesandbox.io/node_modules/dnd-core/lib/cjs/DragDropManagerImpl.js:67:21)
at HTML5Backend.handleTopDragEndCapture (https://xzoq6xprlz.codesandbox.io/node_modules/react-dnd-html5-backend/lib/cjs/HTML5Backend.js:145:31)
``

Workaround

Calling into unmounted components is probably not the right behavior, so I added an if statement in react-dnd/src/decorateHandler.tsx:handleChange that drops the notification if the decorateHandler has already been unsubscribed. diff attached.

diff --git a/packages/react-dnd/src/decorateHandler.tsx b/packages/react-dnd/src/decorateHandler.tsx
index 85385ec..bcd149e 100644
--- a/packages/react-dnd/src/decorateHandler.tsx
+++ b/packages/react-dnd/src/decorateHandler.tsx
@@ -159,6 +159,15 @@ export default function decorateHandler<Props, CollectedProps, ItemIdType>({
                }
 
                public handleChange = () => {
+                       if (this.disposable.isDisposed) {
+                               console.log("in handleChange")
+                               //because redux takes a snapshot of all subscribers to 
+                               //events when it starts dispatch, it is still possible to call into this even after 
+                               //the subscription has been unsubscribed
+                               //to prevent against calling into unmounted objects, return immediately
+
+                               return
+                       }
                        const nextState = this.getCurrentState()
                        if (!shallowEqual(nextState, this.state)) {
                                this.setState(nextState)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[hellofantastic]

@mx2323 was this pushed in? Even if it was the react-sortable-tree still needs latest react-dnd updates, I think its in waiting

[mx2323]

i didnt end up pushing it.

[paillave]

Is there any way to have it working? What is the workaround?