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

New VDOM Rendering Engine #58

Merged
merged 28 commits into from
Aug 16, 2018
Merged

New VDOM Rendering Engine #58

merged 28 commits into from
Aug 16, 2018

Conversation

agubler
Copy link
Member

@agubler agubler commented Aug 9, 2018

Type: feature

The following has been addressed in the PR:

Description:

A new implementation of the VDOM rendering engine.

  • Resolves performance regressions introduced by fixes for supporting inserting nodes in the correct position when a widget returns an array of DNodes.
  • Unrolls the recursive loop used in the previous VDOM implementation to processing a queue of instructions for both the vdom side and the DOM commit.
  • Provides a maintainable platform for future feature and performance enhancements
  • Feature parity with the existing VDOM implementation
  • Removes the requirement for the ProjectorMixin which reduces indirection and ultimately bundle size.
  • Provides a shim ProjectorMixin to ensure backwards compatibility.
  • Automatically detects whether a merge is required, without requiring a separate API.

Performance benchmarks: New vdom (left) vs the existing implementation (right)

interactive_results

Basic Usage

import { renderer } from '@dojo/framework/widget-core/vdom';
import { w } from '@dojo/framework/widget-core/d';
import App from './MyApp';

const r = render(() => w(MyApp, {});
r.mount();

Implementation Tracking

  • Basic widget and node diff'ing and rendering
  • Basic event handlers
  • Basic Render scheduling
  • Support returning arrays
  • Insert before calculation
  • Sub tree rendering and depth ordering
  • Registry support
  • Support deferred properties
  • Auto instance binding for widget and node functions
  • SVG Support
  • Support changing event handlers across renders
  • DOM Merging Support
  • Sync Mode
  • Node operations (focus, blur, click, scrollIntoView)
  • Support dom pragma
  • Lifecycle hooks (onAttach, onDetach)
  • Advanced Render scheduling Not being included as part of these changes
  • Exit, enter and update animation support Update animation support has been removed
  • No Performance Regressions versus existing vdom (improvements)
  • Custom element support
  • Passing Unit Tests
  • README updates
  • Removed console logs for missing keys, issue raised to address general debugging before V4.

Resolves #54

@agubler agubler added enhancement New feature or request next Issue/Pull Request for the next major version labels Aug 9, 2018
@agubler agubler changed the title [WIP] New VDOM Rendering Engine New VDOM Rendering Engine Aug 10, 2018
private _filterAndConvert(nodes: DNode): WNode | VNode;
private _filterAndConvert(nodes: DNode | DNode[]): (WNode | VNode) | (WNode | VNode)[];
private _filterAndConvert(nodes: DNode | DNode[]): (WNode | VNode) | (WNode | VNode)[] {
const isArray = Array.isArray(nodes);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably rename this nodesIsArray so this is clearer

private _filterAndConvert(nodes: DNode | DNode[]): (WNode | VNode) | (WNode | VNode)[];
private _filterAndConvert(nodes: DNode | DNode[]): (WNode | VNode) | (WNode | VNode)[] {
const isArray = Array.isArray(nodes);
const filteredNodes = Array.isArray(nodes) ? nodes : [nodes];
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny one; you could just do isArray rather Array.isArray(nodes) again

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, it doesn't infer the types correctly if you reuse the var.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shame!

const convertedNodes: (WNode | VNode)[] = [];
for (let i = 0; i < filteredNodes.length; i++) {
const node = filteredNodes[i];
if (!node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

node === undefined will probably be faster and clearer here

Copy link
Member Author

Choose a reason for hiding this comment

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

It can also be null 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Right!

node.bind = this;
convertedNodes.push(node);
if (node.children && node.children.length) {
node.children = this._filterAndConvert(node.children);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if recursion is faster than iteration

Copy link
Member Author

Choose a reason for hiding this comment

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

I suspect that the difference is negligible, certainly not shown up as a bottleneck in any of the performance analysis I've done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, more just an open ended question. If we don't spend much processing time here would imagine it won't matter much as you say.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a hot path, runs for the render result of every widget being processed. I can certainly look at unrolling the loop and see if it makes any meaningful different.

// 'creates VNode with the DOM node attached, associated tagname and default diff type'() {
// const div = document.createElement('div');
// const vnode = dom(
// {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need deleting?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, needs un-commenting!

const projector = new Projector();

projector.append();
const r = renderer(() => w(App, {}));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is much more straightforward and easy to understand. Nice 😄

Copy link
Contributor

@JamesLMilner JamesLMilner left a comment

Choose a reason for hiding this comment

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

Overall looking good. End user ergonomics will be simpler and the code reads better in general. 👍

@agubler agubler changed the base branch from next to master August 14, 2018 12:10
@agubler agubler self-assigned this Aug 14, 2018
@agubler
Copy link
Member Author

agubler commented Aug 16, 2018

Tested with js-framework-benchmark and examples - hnpwa, realworld, custom-element-menu, widget-showcase & custom-element-showcase.

Copy link
Member

@tomdye tomdye left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request next Issue/Pull Request for the next major version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Dojo Vdom Rendering Engine
4 participants