What about using TreeWalker and google/incremental-dom?

[patrick-steele-idem]

What about using TreeWalker and google/incremental-dom?

[patrick-steele-idem]

I started developing morphdom before incremental-dom, but took a pretty close look at incremental-dom when it was released.

incremental-dom doesn't use the native TreeWalker API. Instead, they created their own implementation that is similar to the native TreeWalker API: https://github.com/google/incremental-dom/blob/c7fd9017406cdc6a69011664dc592f8e96fa85f2/src/tree_walker.js

I haven't tried the native TreeWalker implementation, but I think it is probably worth checking out. I would be surprised if it made much of a difference compared to just using node.firstChild and node.nextSibling to walk the tree, but I've been surprised by a lot of things related to performance and the browser. It would be tricky to integrate TreeWalker with morphdom because morphdom doesn't visit all nodes through a straight traversal and a single pointer to a current node would make things more difficult (requiring calls to walk back up the tree).

Perhaps what's most important in regard to incremental-dom is that incremental-dom only works when it is used as a compilation target by a template engine. morphdom will work with any templating engine that produces an HTML string. While there might be some benefits to using incremental-dom as a compilation target, I'm not convinced it is needed and that it would justify the complexity. Not to mention, incremental-dom does some crazy stuff with globals since it has no concept of a rendering context.

[ducktype]

Clear, the fact "morphdom doesn't visit all nodes through a straight traversal" make my suggestions about using TreeWalk and incremental-dom not applicable as both need this.

About working with "any templating engine that produces an HTML string" i'm with you, i think is the right choice, but i think incremental-dom can be used also for this purpose:

example code (proof of concept):

var IncrementalDOM = require('incremental-dom');

var range;

function toElement(str) {
    if (!range) {
        range = document.createRange();
        range.selectNode(document.body);
    }

    var fragment;
    if (range.createContextualFragment) {
        fragment = range.createContextualFragment(str);
    } else {
        fragment = document.createElement('body');
        fragment.innerHTML = str;
    }
    return fragment.childNodes[0];
}

var walkDOM = function (node,open_callback,close_callback) {
        open_callback(node);
        node = node.firstChild;
        while(node) {
            walkDOM(node,open_callback,close_callback);
            node = node.nextSibling;
        }
        close_callback(node)
}

function morphdom_using_incrementaldom(fromNode, toNode, options) {
   if (typeof toNode === 'string') {
        toNode = toElement(toNode);
    }
    IncrementalDOM.patch(fromNode,function(){
      walkDOM(toNode,function(curr_node){
         IncrementalDOM.elementOpen(curr_node.tagName, '', null);
        //TODO: handle element attributes
        if (curr_node.nodeType == 'TEXT') {
           IncrementalDOM.text(curr_node.nodeValue);
        }
      },function(curr_node) {
         IncrementalDOM.elementClose(curr_node.tagName);
      })
    })
}

what about this way, it will work?

Will be interesting to see a performance comparison between morphdom and this approach with incremental-dom?

what do you think?

[patrick-steele-idem]

Hi @ducktype, sorry for being slow to respond, but better late than never. Using incremental-dom in the way that you described will work, but it is pretty much guaranteed to be slower since incremental-dom was designed to be a compilation target to avoid creating real DOM nodes. morphdom assumes there is already a real DOM that the existing DOM should be transformed to match and the code is very focused on that use case. I don't know how much slower incremental-dom will be when used in the way you described, but if you are curious then I would suggest adding it as a benchmark by updating the following file: https://github.com/patrick-steele-idem/morphdom/blob/master/test/browser/benchmark.js

[shock01]

Implemented this in a different repository and used morphdom via browserify to create a benchmark.
Using TreeWalker 100 iterations: 3.8ms avg, Using Morphdom : 4.7ms avg on my local machine.
Also note that in my implementation the fixture large doesn't have equality for isEqualNode when comparing the result of morphdom to the actual to.html. This can be due to either browser version or the browserify stuff or maybe me wrongly copying templates.

I could not get the same numbers out of the benchmark that patrick has done due to the fact that this module doesnt work locally using npm install

[patrick-steele-idem]

Hey @shock01, I suspect the performance difference is mostly associated with the different feature set and not the use of TreeWalker. I did look at using TreeWalker and the beginning, and while I don't remember all of the details, it didn't look like a good option at the time. morphdom has extra overhead because it matches up DOM elements by id (this is very important in some applications).

Also note that in my implementation the fixture large doesn't have equality for isEqualNode when comparing the result of morphdom to the actual to.html. This can be due to either browser version or the browserify stuff or maybe me wrongly copying templates.

You don't specify which implementation is in the wrong. morphdom has a fairly extensive test suite and while we don't use isEqualNode we do test for equality using the following code: morphdom/test/browser/test.js

I could not get the same numbers out of the benchmark that patrick has done due to the fact that this module doesnt work locally using npm install

I have no issues running the tests and benchmarks locally using the following steps:

git clone https://github.com/patrick-steele-idem/morphdom.git
cd morphdom
npm install
npm test
npm run benchmark

[shock01]

Read the comparison.
I did two comparisons:
eg: isEqualNode and also a stringdiff on outerHTML and both failed.
That suspects me that the DOM outcome is actually different because i use native DOM method.
Thus it returns the results as interpreted by the browser while yours interprets the results as a string result which you build up on your own.

I attached the npm log because locally it fails on npm install (windows)

npm-debug.zip

[shock01]

new XMLSerializer().serializeToString(element) also fails on comparison

[patrick-steele-idem]

Hey @shock01, the npm scripts configured for the package are not compatible with Windows, but you should be able to run the corresponding commands directly:

morphdom/package.json

Lines 5 to 16 in 45bb5be

	"scripts": {
	"test": "npm run test-browser && npm run lint",
	"benchmark": "npm run benchmark-browser",
	"all": "npm run all-browser && npm run lint",
	"lint": "jshint lib/",
	"test-browser": "node test/mocha-phantomjs/run.js test",
	"benchmark-browser": "node test/mocha-phantomjs/run.js benchmark",
	"all-browser": "node test/mocha-phantomjs/run.js test benchmark",
	"mocha-phantomjs": "node test/mocha-phantomjs/run.js",
	"mocha-phantomjs-run": "mocha-phantomjs ./test/mocha-phantomjs/generated/test-page.html",
	"prepublish": "mkdir -p dist && node_modules/umd/bin/cli.js morphdom lib/index.js -c > dist/morphdom-umd.js"
	},

However, it looks like you can't do a local npm install due to the prepublish script. I didn't realize prepublish runs on npm install, but according to the docs:

prepublish: Run BEFORE the package is published. (Also run on local npm install without any arguments.)

I recommend removing the prepublish script on your machine.

I don't have Windows installed and I have no bandwidth to make morphdom work with a local npm install on Windows, but if you want to give it a shot that would be appreciated.

If two different DOM node is serialized out to a string using the DOM API, then that seems good enough to me that they are in fact interpreted the same by the browser. That is, of course, the serialization is implemented correctly (which I am verifying...).

Also, probably better to open up a separate Github issue for discussion on this issue.

[shock01]

Thnx for the feedback will try later on