phosphorjs / phosphor

The PhosphorJS Library
BSD 3-Clause "New" or "Revised" License
1.04k stars 166 forks source link

Adds a "pass thru" virtual element #437

Open telamonian opened 4 years ago

telamonian commented 4 years ago

Fixes #395, #436, and probably a bunch of other issues.

This PR adds the hpass function and VirtualElementPass class described in #436. There are still a number of unanswered questions as to the detials of their implementations. For example, should VirtualElementPass allow for children and, if so, how should they be handled?

telamonian commented 4 years ago

This PR is now functional, and at a point where I could really use some feedback. Here's what hpass currently looks like:

https://github.com/phosphorjs/phosphor/blob/192025a39249a253a354d22a9ecc425bc1463abf/packages/virtualdom/src/index.ts#L960-L962

hpass will create a vdom node just like h does, but it doesn't allow children. Instead, the node's children will be created/managed by the render callback. This ensures that nodes added by render won't have any immediate siblings or children. It's set up this way since React (and presumably other vdom libs) doesn't play well with DOM nodes that it didn't create.

The big remaining issue seems to be cleanup and avoiding memory leaks. For example, I modified Title and TabBar to allow for passing in an icon rendering callback via a new Title.iconRender parameter. The changes I'm working on in jupyterlab/jupyterlab#7299 make use of iconRender to render document tab icons via a call to ReactDOM.render. If I profile memory use while creating and closing documents tabs, the underlying React objects seemingly don't ever get cleaned up.

The specific issue here seems to be that, in order to cleanup the effects of ReactDOM.render(someNode), you need to call ReactDOM.unmountComponentAtNode(someNode) (and I think you need to call it before you detach someNode from the DOM, but I'm not 100% on that). I could definitely use some suggestions on how to hook this kind of cleanup into the current Phosphor vdom/Widget renderers.

cc @blink1073 @sccolbert

telamonian commented 4 years ago

Turns out the cleanup issue was easier to fix than I had anticipated. In virtualdom, now whenever updateContent removes a node from the DOM, that node and it's children are recursively checked. If any of those nodes node were created via a vNode of type VirtualElementPass, vNode.renderer.unrender(node) will get called, allowing a user of hpass to do any needed cleanup via an extra callback.

The only downside of this approach is that now a user of hpass needs to supply two callbacks, both a render and an unrender. Having some kind of cleanup function does seem to be absolutely necessary to avoid memory leaks with React.

I tested the changes in this PR, and it appears that the memory leak has indeed been resolved.

That's the last major issue (that I'm aware of) resolved, so this PR is ready for review. I'll move this it out of [WIP].

telamonian commented 4 years ago

In order to get a better grasp of how exactly vdom is getting cleaned up, I ended up doing a much more extensive round of memory profiling and tracking of rendered React components (via the React Developer Tools). The bottom line is that the current approach to cleanup in this PR seems to work quite well.

On the down side, there do seem to be at least a couple of edge cases in jlab itself where vdom-managed DOM nodes aren't getting cleaned up correctly. For example, if you close your last open document tab, the tab's underlying DOM never gets a final pass through updateContent in order to properly clear it.

sccolbert commented 4 years ago

I'll want to weight in. Please don't merge until I have.

telamonian commented 4 years ago

@blink1073 I remembered the unittests, but I forgot the docstrings. They've now all been added in.

@sccolbert Please do! I would love to get your feedback