Simplified/improved custom rendering of virtual nodes: removed hpass and VirtualElementPass, added optional renderer param

[telamonian]

This PR contains a massive simplification and a number of improvements to the implementation of virtual nodes with custom renderers (formerly known as "passthru" virtual nodes) in the virtualdom pkg.

Ideally this PR will get pulled in sometime in the next week. so that I can incorporate its improvements into jlab before the 2.0 release, since it may involve some breaking changes in jlab (see note bellow about breaking changes w.r.t. lumino).

Overview of changes:

• I removed VirtualElementPass and hpass and folded their implementations into VirtualElement and h. A custom renderer (what I had formerly been calling a "passthru" renderer) can now be set on any virtual node via an optional arg to either VirtualElement or h. This has a number of benefits:

  • reduced the number of runtime type checks needed in the virtualdom
    implementation
  • no one liked the "pass" or "passthru" names. All uses of such have now been removed
  • we're now back to having only two virtual element types (and one is super simple and just for text). As well, the notion of setting a custom renderer as an optional argument is simpler than having a separate special "escape hatch" virtual element type. This should make the custom renderer stuff easier to pick up, learn, and use

• renderer is now an optional property for both h and new VirtualElement. Since renderer is optional, backwards compatibility is preserved for these functions.

• it is now possible for a node to have both a custom renderer and children.When node.renderer.render is called, the node's .attr and .children fields will be passed as options. The render function will then be able to create actual DOM children based on the passed .children field. If the particular render function being used does not support .children, it will just be ignored

• I fixed up and added to all of the related docstrings, with the goal in mind of making it easier to understand what a VirtualElement.IRenderer actually is and does. Hopefully that will make it easier for people to create and use their own custom element renderers

Technically, this PR includes breaking changes in the form of the removal of hpass and VirtualElementPass. However, a search of github reveals that there is currently 0% adoption of either hpass or VirtualElementPass (aside from my own work in jlab core), so I think this will be safe to pull in.

update

This PR now also includes a fix for a minor problem with tabbar icons. Previously, when title.iconRenderer was set, title.iconLabel would get set as the title attribute of the icon's DOM element. This was not correct.

Instead, .iconLabel will now be treated the same regardless of the presence of .iconRenderer: .iconLabel will be set as a child of the virtual element corresponding to the icon. If .iconRenderer is set, it's then the responsibility of .iconRenderer.render whether and how to create the actual DOM corresponding to .iconLabel.

[telamonian]

Here's what's left for WIP:

• add fixes to widgets (title and tabbar)
• fix unittests in virtualdom pkg

The PR is now done!

@blink1073 @vidartf (or anyone else who feels like it): can you please review? I'd like to get this pulled in so that I can add some relevant changes to jlab before the 2.0 release.

There's a minor stylistic flaw that I'm unsure how to resolve. There doesn't seem to be a good way to add the renderer param to the same position in the signatures of both VirtualElement.constructor (in which renderer is last, after children) and h (in which renderer is between attrs and children):

lumino/packages/virtualdom/src/index.ts

Line 754 in 6282805

constructor(tag: string, attrs: ElementAttrs, children: ReadonlyArray<VirtualNode>, renderer?: VirtualElement.IRenderer) {

lumino/packages/virtualdom/src/index.ts

Lines 843 to 846 in 6282805

	export function h(tag: string, ...children: h.Child[]): VirtualElement;
	export function h(tag: string, attrs: ElementAttrs, ...children: h.Child[]): VirtualElement;
	export function h(tag: string, renderer: VirtualElement.IRenderer, ...children: h.Child[]): VirtualElement;
	export function h(tag: string, attrs: ElementAttrs, renderer: VirtualElement.IRenderer, ...children: h.Child[]): VirtualElement;

Previously, there was a nice symmetry between the signatures of VirtualElement and h:

lumino/packages/virtualdom/src/index.ts

Line 745 in 5041edd

constructor(tag: string, attrs: ElementAttrs, children: ReadonlyArray<VirtualNode>) {

lumino/packages/virtualdom/src/index.ts

Lines 846 to 847 in 5041edd

	export function h(tag: string, ...children: h.Child[]): VirtualElement;
	export function h(tag: string, attrs: ElementAttrs, ...children: h.Child[]): VirtualElement;

Basically, I can't put renderer last in the h sig (the ...children arg uses the spread operator, and so has to go last), and I can't put renderer anywhere but last in VirtualElement (since all its args are positional, and rearranging any would break backwards compatibility). I'm unsure if new VirtualElement is ever called outside of the virtualdom pkg, so maybe the solution would be to just go ahead and rearrange the sig for that?

Another possibility would be use a custom argument resolution scheme in VirtualElement, like we currently do in h. However, I'm unsure in the first place as to why h has such a complex signature, so it's hard to judge if that would be a good idea. Also, adding a bunch of runtime type checks to something that gets called as often as VirtualElement seems like an antipattern in terms of performance.

Some external input on the issue would be most appreciated

[telamonian]

I looked, and to my surprise there do appear to be 4 external projects that depend on the exact signature of VIrtualElement.constructor. Interestingly, only one of them is javascript. Two are scala (by way of scala.js), and the other is F# of all things.

On the other hand, there's a ton of projects that import VirtualElement to use as a type (mostly as part of defining a new renderer for a Widget). Direct calls to new VirtualElement are rare on purpose: by convention you're supposed to use h to construct VirtualElement instances. It's beyond the scope of this PR, but would it make sense to split VirtualElement into both an interface and a class, and then make the implementation (ie the class) private?

[blink1073]

I think we should save API changes for a major release of the virtualdom package, for jlab 3.0.

[telamonian]

I think we should save API changes for a major release of the virtualdom package, for jlab 3.0.

Which API changes do you mean?

1. Removal of hpass and VirtualElementPass

2. Addition of optional renderer param to h and VirtualElement.constructor

3. Addition of optional options param to IRenderer.render, and the change of IRenderer.unrender to optional

The removals in 1) were technically breaking, which yeah, even if nobody's using them probably still isn't a great idea. I just added a backwards compatibility shim that provides them.

Edit

Ohhh, you were responding to my comment about splitting VirtualElement into a public interface and a private implementation. Whoops...

[blink1073]

Thanks @telamonian, this is a nice enhancement and cleanup. I left one comment. I am traveling this morning, but should be able to get this merged and released later today.

[blink1073]

Thanks!

[blink1073]

Published @lumino/virtualdom@1.5.0

[telamonian]

Sweet. Thanks @blink1073!