-
Notifications
You must be signed in to change notification settings - Fork 168
Adds a "pass thru" virtual element #437
base: master
Are you sure you want to change the base?
Conversation
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
requires the changes in the phosphorjs/phosphor#437 PR
…ring still need to figure out how to handle cleanup; current implementation likely causes a memory leak due to uncleaned-up React components
This PR is now functional, and at a point where I could really use some feedback. Here's what phosphor/packages/virtualdom/src/index.ts Lines 960 to 962 in 192025a
The big remaining issue seems to be cleanup and avoiding memory leaks. For example, I modified The specific issue here seems to be that, in order to cleanup the effects of |
Turns out the cleanup issue was easier to fix than I had anticipated. In The only downside of this approach is that now a user of 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]. |
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 |
still getting some unittest failures in FocusTracker and TabBar in Widgets
There was a problem hiding this 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.
I'll want to weight in. Please don't merge until I have. |
@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 |
Fixes #395, #436, and probably a bunch of other issues.
This PR adds the
hpass
function andVirtualElementPass
class described in #436. There are still a number of unanswered questions as to the detials of their implementations. For example, shouldVirtualElementPass
allow for children and, if so, how should they be handled?