Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

Adds a "pass thru" virtual element #437

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

telamonian
Copy link

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?

issues with vdom siblings still remain. siblings will get clobbered if/when
the vdom updates. Still works well enough when the hpass element is the
first sibling
the value of `iconPass` is used to initialize an hpass vdom element
in eg the tabbar renderer
…ndling

accomplished this in part by moving a bunch of the complexity to the
creatDOMNode function
telamonian added a commit to telamonian/jupyterlab that referenced this pull request Oct 13, 2019
…ring

still need to figure out how to handle cleanup; current implementation likely
causes a memory leak due to uncleaned-up React components
@telamonian
Copy link
Author

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

export function hpass(render: VirtualElementPass.IRender, tag: string, attrs: ElementAttrs = {}): VirtualElementPass {
return new VirtualElementPass(render, tag, attrs);
}

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
Copy link
Author

telamonian commented Oct 14, 2019

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 telamonian marked this pull request as ready for review October 14, 2019 20:25
@telamonian
Copy link
Author

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.

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than a few missing docstrings, this looks great to me, thanks @telamonian! I'll leave this open for a bit in case others want to weigh in.

packages/virtualdom/src/index.ts Show resolved Hide resolved
packages/widgets/src/title.ts Show resolved Hide resolved
packages/widgets/src/title.ts Show resolved Hide resolved
@sccolbert
Copy link
Member

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

@telamonian
Copy link
Author

telamonian commented Oct 15, 2019

@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

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

Successfully merging this pull request may close these issues.

Create VirtualElement from React node
3 participants