New VDOM Rendering Engine by agubler · Pull Request #58 · dojo/framework

agubler · 2018-08-09T18:01:12Z

Type: feature

The following has been addressed in the PR:

There is a related issue
All code has been formatted with prettier as per the readme code style guidelines
Unit or Functional tests are included 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)

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

Resolves #54

JamesLMilner · 2018-08-13T21:17:48Z

+	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);


I would probably rename this nodesIsArray so this is clearer

JamesLMilner · 2018-08-13T21:17:51Z

+	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];


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

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

JamesLMilner · 2018-08-13T21:20:22Z

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


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

It can also be null 😄

JamesLMilner · 2018-08-13T21:22:22Z

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


I wonder if recursion is faster than iteration

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

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.

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.

JamesLMilner · 2018-08-13T21:27:35Z

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


Does this need deleting?

Thanks, needs un-commenting!

JamesLMilner · 2018-08-13T21:28:14Z

-const projector = new Projector();
-
-projector.append();
+const r = renderer(() => w(App, {}));


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

JamesLMilner

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

…ction for an invalidation

… deferred props

…n appended

…he correct position

…erting render

agubler · 2018-08-16T13:01:06Z

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

tomdye

LGTM

agubler added enhancement New feature or request next Issue/Pull Request for the next major version labels Aug 9, 2018

agubler force-pushed the vdom branch from 04bf4ea to b1f6014 Compare August 9, 2018 18:21

agubler changed the title ~~[WIP] New VDOM Rendering Engine~~ New VDOM Rendering Engine Aug 10, 2018

JamesLMilner reviewed Aug 13, 2018

View reviewed changes

agubler changed the base branch from next to master August 14, 2018 12:10

agubler self-assigned this Aug 14, 2018

agubler force-pushed the vdom branch from 7a6f7bc to 7e43a85 Compare August 15, 2018 00:31

agubler added 16 commits August 16, 2018 13:05

Initial vdom rewrite

38a7e8a

refactor

109c7ce

vdom

7cf0a1a

Additional vdom changes

9ec2bc8

More changes to vdom to support merges correctly

b5878a7

Refactors

ea146bb

More refactors

d4628a9

Fix dom vnode logic and refactor property/attribute/event updates

41290a0

more refactors

14313fe

Use original properties passed to the widget when creating the instru…

89632b4

…ction for an invalidation

Do not try to merge for custom elements

58cab92

Move dom application to after the invalidation queue has been processed

2febbd4

Run deferred properties in WidgetBase

eaed68d

README

c61fb93

Clean up and tests for ProjectorMixin

25aaeff

Move invalidate back to instanceData

5a6939a

agubler added 12 commits August 16, 2018 13:05

Uncomment tests

53a119a

Move from class to function for vdom

370b0d8

vdom reorg and tweaking

ffbe3fc

README

bb3d7ce

Add back remaining vdom unit tests

2356978

refactor and fixes to ensure the latest wrapper is used

567388f

Run filtering before clearing the node handler to allow meta usage in…

79da95d

… deferred props

Move registry resolution to WidgetBase from vdom

0663842

Create attach application for operations on a widget after it has bee…

1118e5f

…n appended

Fix insert before calculation to ensure results of widgets added to t…

737ed87

…he correct position

Falsy dnodes are now filtered by widgetbase, so deal with this in ass…

9c770b7

…erting render

more tidying

f0b9aba

agubler force-pushed the vdom branch from bc0659c to f0b9aba Compare August 16, 2018 12:05

matt-gadd approved these changes Aug 16, 2018

View reviewed changes

tomdye approved these changes Aug 16, 2018

View reviewed changes

agubler merged commit b8a3dd8 into dojo:master Aug 16, 2018

agubler mentioned this pull request Aug 29, 2018

In my widget, Vnode meta node information cannot be fetched when Vnode is child to a widget #36

Closed

Conversation

agubler commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Basic Usage

Implementation Tracking

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesLMilner left a comment

Choose a reason for hiding this comment

Uh oh!

agubler commented Aug 16, 2018

Uh oh!

tomdye left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

agubler commented Aug 9, 2018 •

edited

Loading