Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React fails to unmount component from within event handler #3298

Closed
slorber opened this issue Mar 2, 2015 · 14 comments
Closed

React fails to unmount component from within event handler #3298

slorber opened this issue Mar 2, 2015 · 14 comments

Comments

@slorber
Copy link
Contributor

slorber commented Mar 2, 2015

Hi,

When trying to unmount my whole app, I got some error.

Uncaught TypeError: Cannot read property 'firstChild' of undefined
ReactMount.js:606ReactMount.findComponentRoot
ReactMount.js:606ReactMount.findReactNodeByID ReactMount.js:552getNode
ReactMount.js:128executeDispatch EventPluginUtils.js:109SimpleEventPlugin.executeDispatch
SimpleEventPlugin.js:305forEachEventDispatch EventPluginUtils.js:95executeDispatchesInOrder
EventPluginUtils.js:119executeDispatchesAndRelease EventPluginHub.js:46forEachAccumulated
forEachAccumulated.js:25EventPluginHub.processEventQueue
EventPluginHub.js:251runEventQueueInBatch
ReactEventEmitterMixin.js:18ReactEventEmitterMixin.handleTopLevel
ReactEventEmitterMixin.js:44handleTopLevelImpl ReactEventListener.js:80Mixin.perform
Transaction.js:134ReactDefaultBatchingStrategy.batchedUpdates
ReactDefaultBatchingStrategy.js:66batchedUpdates
ReactUpdates.js:109ReactEventListener.dispatchEvent ReactEventListener.js:175

I think it's not a big deal.

According to what I see with the debugger, it seems to be because a SyntheticMouseEvent is trying to get dispatched. And I guess the target has just been unmounted...

Note that my use case looks like this:

var Hello = React.createClass({
    render: function() {
        return <div onClick={unmount}>Hello {this.props.name}</div>;
    }
});

When using an unmount synchronous implementation, I get this error.
When adding a setTimeout 0 in the unmount code, I got no error.

I could not reproduce this in a jsfiddle, but I guess it's probably because I don't really know how batching work in React.

@slorber
Copy link
Contributor Author

slorber commented Mar 2, 2015

You can find more informations on my usecase here:

#3038

Basically on user language change I want to add the new language to the React context, and force re-render of the whole thing.

This is not Flux code but I guess you'll understand what it does:

context.addEventListener(function(event) {
    if ( event.name === AppEvents.Names.USER_PREFERRED_LANGUAGE_SELECTED ) {
        setTimeout(function() {
            var newLanguage = event.data.language;
            var newReactContext = buildReactContext(newLanguage);
            context.unmount();
            context.setReactContext(newReactContext);
            context.renderCurrentAtomState();
        },0);
    }
});

As you can see it works for me with a setTimeout, I get an error only when unmounting synchronously.
(This really happens during the unmount, if I remove the other calls, I still get the error)

@syranide
Copy link
Contributor

syranide commented Mar 2, 2015

Unmounting during a React event is not currently supported AFAIK.

@jimfb
Copy link
Contributor

jimfb commented Mar 2, 2015

Unmounting as the result of a event sounds like a perfectly reasonable thing to want to do; I think we should try to support this.

@slorber can you create a simple jsfiddle that demonstrates the issue?

@jimfb jimfb changed the title Little bug when unmounting React fails to unmount component from within event handler Mar 2, 2015
@slorber
Copy link
Contributor Author

slorber commented Mar 2, 2015

@jimfb Yes I've successfully reproduced the case here: http://jsfiddle.net/wevohwfp/

My first attempt was not working because I did not try to add another event listener to the parent

@jimfb
Copy link
Contributor

jimfb commented Mar 2, 2015

Awesome, thanks!

Issue is related to mutating nodes synchronously within an event loop. We may want to make unmount take effect at the end of the event loop. I think this demonstrates the more general problem of how to handle these top-level functions from within an event loop.

@jasonslyvia
Copy link
Contributor

plus one here, it's reasonable behaviour to unmount a component as the result of a React event.

@blairanderson
Copy link

I just ran into this.

for anyone else coming here, stopPropagation on the event does not work either.

I ended up building a wrapper component to handle, basically acting as a mini router for the user-flow.

@bramschulting
Copy link

We encountered this problem as well, but we've found a dirty work-around. If you wrap React.unmountComponentAtNode(target) in a setTimeout, this will work.

We use this in a portal mixin, which renders a React component in a different DOM element. This raised another problem when rendering a new component directly after unmounting the first one. In that case, the second component won't show up. We fixed this by storing a unique key in the component state and the DOM element, and only calling unmountComponentAtNode if these keys are the same.

setTimeout(()=> {
  if (this.state.portalKey === this._target._portalKey) {
    React.unmountComponentAtNode(this._target);
  }
});

This works for now, but we'd love a less dirty, more native solution

@lencioni
Copy link
Contributor

Is this perhaps a duplicate of #2605?

@sophiebits
Copy link
Collaborator

This now throws with v15 (http://jsfiddle.net/wevohwfp/1/):

Invariant Violation: React DOM tree root should always have a node reference.

I'll note that this does work if you unmount a subtree but apparently not if you unmount the whole root – because unmounting is batched differently from updates.

@quicksnap
Copy link

I tried running the following test but could not produce the error: https://gist.github.com/quicksnap/10905082804338c9ac3f5db474dd303e

I'm guessing ReactTestUtils.SimulateNative.click isn't appropriate in this situation. Is there an existing pattern in the test suite that could reproduce?

@sophiebits
Copy link
Collaborator

@quicksnap Not sure why that wouldn't work. You could try Simulate.click which is generally preferred unless you're trying to test the event system code.

@aweary
Copy link
Contributor

aweary commented Apr 21, 2017

This appears to work out of the box with Fiber: https://jsfiddle.net/5rg0wu8c/

@aweary aweary closed this as completed Apr 21, 2017
@plinehan
Copy link

plinehan commented Apr 5, 2018

For any other n00bs, Fiber is React v16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests