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

ReactElementWidget fails to unmount React element from the DOM #5766

Closed
quigleyj-mavenomics opened this issue Dec 14, 2018 · 2 comments · Fixed by #5479
Closed

ReactElementWidget fails to unmount React element from the DOM #5766

quigleyj-mavenomics opened this issue Dec 14, 2018 · 2 comments · Fixed by #5479

Comments

@quigleyj-mavenomics
Copy link

@quigleyj-mavenomics quigleyj-mavenomics commented Dec 14, 2018

Describe the bug

We have a few React elements wrapped up in ReactElementWidget, as does the JupyterLab code base. One of them is particularly heavyweight, which helped surface this issue. Because ReactElementWidget makes no call to ReactDOM.unmountComponentAtNode(), the react elements rendered using this wrapper widget never get fully cleaned up.

To Reproduce

I have a minimal example of the issue here:

https://codesandbox.io/s/pw8qr9lv7j

(nb. React Dev Tools doesn't seem to like the iframe, if you open the output in a new window then you will be able to inspect the React DOM)

If you have React Dev Tools, you can see the <div>. When you click on the "Hi" text, it disposes itself and cleans the React node up. However, if you uncomment the line unmountComponentAtNode(this.node); (index.tsx:13), then when it gets disposed, the react element hangs around in memory.

Expected behavior

I'd expect it would be disposed as part of #dispose(), either in ReactElementWidget or the VDomRenderer. While it's possible for subclassers to do it, it opens up a leaky abstraction if multiple ReactDOM instances are loaded on the page (you need to get the ReactDOM instance that @jupyterlab/apputils is using).

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 15, 2018

This is one of the big points addressed by #5479.

@quigleyj-mavenomics
Copy link
Author

@quigleyj-mavenomics quigleyj-mavenomics commented Dec 15, 2018

Awesome, thanks for following up! Looks like that will get handled in ReactWidget#onBeforeDetatch(), which is probably a better place than dispose.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants