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

Adds a "pass thru" virtual element #29

Merged
merged 18 commits into from
Dec 17, 2019

Conversation

telamonian
Copy link
Member

@telamonian telamonian commented Dec 14, 2019

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

This PR adds the hpass function and VirtualElementPass class described in phosphorjs/phosphor#436.

This PR was originally phosphorjs/phoshor#437. @blink1073 Since you reviewed the original PR, could you please take a look at this one too?

@telamonian
Copy link
Member Author

Here's the original motivation from phosphorjs/phosphor#436 (comment):

Following up on a suggestion made by @jasongrout during the latest Phosphor session/call, I think we should add a "pass thru" vdom element that renders a real DOM element using a callback. This could act like an escape hatch from Phosphor's vdom and allow for composition with vdom elements from other libraries (like React, hint, hint).

Given the structure of the virtualdom package and the updateContent function, I think it would make sense to implement this via an hpass function with the following signature:

export function hpass(callback: (host: HTMLElement) => void): VirtualElementPass {
 ...
}

This could exist alongside the extant h function. During updateContent, when a node with:

node.type === 'passthru'

is encountered, node.callback(host) will be invoked, and from that point on any diffing/updating will be the responsibility of the callback.

I think the most important/urgent use case for this is allowing for the blending of React vdom into Phosphor vdom. The above pattern can mesh very nicely with ReactDOM.render(host, Component), since that function does its own diffing. This would allow Phosphor to better accommodate React while still avoiding any dependency on it.

@telamonian
Copy link
Member Author

telamonian commented Dec 14, 2019

And here's a summary of the contents of the PR from phosphorjs/phosphor#437 (comment):

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

/**
* Create a new "pass thru" virtual element node.
*
* @param tag - The tag name for the parent element.
*
* @param attrs - The attributes for the parent element, if any.
*
* @param renderer - an object with render and unrender functions, if any.
*
* @returns A new "pass thru" virtual element node for the given parameters.
*
*/
export function hpass(tag: string, renderer?: VirtualElementPass.IRenderer): VirtualElementPass;
export function hpass(tag: string, attrs: ElementAttrs, renderer?: VirtualElementPass.IRenderer): VirtualElementPass;
export function hpass(tag: string): VirtualElementPass {

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.

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
…ring

still need to figure out how to handle cleanup; current implementation likely
causes a memory leak due to uncleaned-up React components
still getting some unittest failures in FocusTracker and TabBar in Widgets
Copy link
Member

@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.

Thanks for updating the signature to match h(), nice symmetry.

@blink1073 blink1073 merged commit 0e115eb into jupyterlab:master Dec 17, 2019
@blink1073
Copy link
Member

Released @lumino/virtualdom@1.3.0

@telamonian
Copy link
Member Author

Released @lumino/virtualdom@1.3.0

Awesome. That's going to save me a lot of fiddling with yarn link. Thanks Steve!

if (title.iconRenderer) {
return hpass('div', title.iconRenderer);
} else {
return h.div({className}, data.title.iconLabel);
Copy link
Member

Choose a reason for hiding this comment

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

If the className is completely disregarded when title.iconRenderer is set, then maybe this.createIconClass(data) should not be called at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

@vidartf You're right, this is inconsistent, and kind of a holdover from an earlier point in the PR. I thought about it for a bit, and what I think should be done is that the className and label should be passed to the hpass element as well, to be set on its parent element. There's a PR for it at #36

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