No Recursive Step - Potential Cause for Failing Key Tests

[JRJurman]

When stepping through the logic for this test
https://github.com/tunnckoCore/demo-minmorph/blob/d587917be0f6e6ae2430a85082fd55b4899758a3/src/app/test-keyed.js#L148-L159

I noticed that we swap a larger chunk of the dom than we need to.
Specifically, here

We dump all of futureStartNode in the parentNode, and then later we pop off the second node.
In reality, I think we need to recurse down the tree more, and then only swap the TEXT nodes. There doesn't appear to be any recursive step - like in nanomorph, here:
https://github.com/choojs/nanomorph/blob/ccbffcf1f0138a0293f7c71ee6521c7640df7173/index.js#L121-L132

[tunnckoCore]

Exactly. I was thinking about that and tried few things locally.

[tunnckoCore]

Hey @JRJurman, @WebReflection!

Heads up! 🎉 Code and tooling is now more organized, cleaned up and linted.
Just yarn install and then yarn test or yarn bench.

There are only 5 assertions that fails, which represents 2 of the keyed tests. And it's probably because that recursion problem.

@WebReflection, do you have any ideas for support such thing in domdiff?
Currently I copy-pasted it here, because the need for same() check instead of just === in some places.

I marked the failing tests with // TODO: PRs welcome for more easier review.

Any ideas and thoughts about the speed and tests are welcome! 🐧

[WebReflection]

@olstenlarck I've no idea what is this about, or what you are asking me. Are you using domdiff? Did you file a bug in domdiff for whatever problem you have encountered?

[tunnckoCore]

No, i didn't open issues there, but i will in some point. @JRJurman descibed well what we are talking about and i don't know what to add more to that. I'm not sure where the recursion should happen too.

I just can't do currently a demo only using domdiff. As about this repo, it's a refactored nanomorph with using domdiff for the childrens diffing instead.

The src/domdiff.js is exactly the same as the latest domdiff 0.4.2, except changes in this, this and this check, to checks for more than == - e.g. node.isSameNode, node.nodeName equality, keys and etc - found here. I'll open issue there to suggest adding such ability.

[JRJurman]

@olstenlarck I can work on those changes on domdiff and we can pull in that fork (until/if the PR gets merged in)

[JRJurman]

Okay, I'm in the process of including an extra parameter that allows for a custom isSameNode function, which defaults to '=='.

However I noticed this comment by WebReflection
choojs/nanomorph#85 (comment)

domdiff also wouldn't care less about patches and it works linearly with a list of nodes, it's not suitable for nested nodes too, just lists of the same parent node.

I believe this is the root cause of this issue

[WebReflection]

I think 3 calls to same(...) shouldn't be a huge issue, or a relevant performance impact, but I don't like much the idea of adding an extra parameter to that signature, since there are already two optionals at the end, one of which is already a callback.

I'll think about releasing a major version with breaking signature changes that will probably look like this:

futureNodes = domdiff(
  parentNode,     // where changes happen
  currentNodes,   // Array of current items/nodes
  futureNodes,    // Array of future items/nodes (returned)
  extras = {
    compareNode: (a, b) => a === b,
    getNode: node => node,
    beforeNode: null
  }
);

would that work for you needs?

[tunnckoCore]

I believe this is the root cause of this issue

@JRJurman, exactly. The problem is that i don't know where the recursion can (if can) happen - i'm not so deep into this. It would be enough to allow calling some callback where it detects that there is some more child nodes on some of the items on the list.

I'll think about releasing a major version with breaking signature changes that will probably look like this:

@WebReflection, sounds good yea. Too much params aren't good thing, so start extras is good way.

would that work for you needs?

I think so, partially. The compareNode name sounds good or probably plural - compareNodes?

In any way, it's good start :)

[WebReflection]

domdiff 1.0 is out

The signature has been overly-simplified as such:

futureNodes = domdiff(
  parentNode,     // where changes happen
  currentNodes,   // Array of current items/nodes
  futureNodes,    // Array of future items/nodes (returned)
  options         // optional object with one of the following properties
                  //  before: domNode
                  //  compare(generic, generic) => true if same generic
                  //  node(generic) => Node
);

The default for {node: generic => Node} is {node: o => o} while the default for compare() is {compare: (a, b) => a == b}.

Let me know ho that goes.

[tunnckoCore]

Sweeet! 🎉 I'll try it out.

But the recursion is still the thing. Any ideas of how can be done? How you handle such cases in hyperhtml where some of the list items has child nodes?

I'm not trying to compete, i'm trying to learn and help nanomorph.

[WebReflection]

You can check the asNode function inside Update.js shared object. Components are all based on children by their own.