Remove parent as 1st mandatory argument for patch

[JSteunou]

Having parent as first argument works very well when the all app is designed and wrote with picodom but not that much when used in a mixed libs app. (yes it happens in real world)

You cannot then just render / create a component to get its main node and then append it manually to a parent node of your choice at the time you choose.

To get around this you have to create a fake parent with document.createElement but that add a useless layer.

I'm also not a big fan of that kind of dependency injection and lake of separation which introduce a dependence between the child and its parent, I do prefer the other way, parent knowing its children, but I think this is just a matter of taste.

[estrattonbailey]

Actually hitting the same road block just this morning 😛

I'm working on a localized stateful component using picodom and the assumption that parent.children[0] is the root of the app from patchElement does make it tricky. My current solution essentially runs a normal render on the first pass, and then subsequent state updates are "rendered" like this:

  // this._self - previous vdom node
  // this.render() - abstracted render fn
  // this.ref - outer DOM node of the component
  const temp = document.createElement('div')
  temp.appendChild(this.ref.cloneNode(true))
  patch(temp, this._self, (this._self = this.render(this.props, this.state)))
  const next = temp.childNodes[0]
  this.ref.parentNode.replaceChild(next, this.ref)
  this.ref = next

Not ideal, though I haven't run any benchmarks to really understand the performance impact here. It may be negligible in the grand scheme of things.

Would it be possible to expose a subset of the patchElement function that is reused internally and could be used externally to diff subtrees?

[jorgebucaran]

Thanks, @JSteunou and @estrattonbailey for bringing this up. Do you have any proposed alternatives? 🤔

[estrattonbailey]

Looking into it, seems like a fun challenge. I've pulled down this repo and linked it to my current project but my experience writing VDOM reconcilers is... well it's zero, lol. Will report back if I have a suggestion.

[JSteunou]

I really like patch(node, newVNode) returning node, seems the most simple from a user perspective, and the most I dont care about vdom, just patch my node

[jorgebucaran]

Can you be more exhaustive? What are the types of those arguments you wrote there? Where is the container?

[JSteunou]

no container, patch is no mount, patch just patch. You give it a true DOM node and it patches it. If you give it an empty div node and the vnode is h('div', {className: 'white big', id: 'foo'}) then the node given as 1st argument will be patched to get those props.

The framework above is in charge of mounting, before or after patch 1st call.

[estrattonbailey]

@jorgebucaran I submitted a PR over at #91 introducing a mount() export and slightly altering the patch() export

[jorgebucaran]

@JSteunou In hyperapp/picodom "terminology", nodes refer to virtual nodes and elements to svg/html elements (Element, HTMLElement, etc.)

I know there is a Node class, but I try to avoid talking about it in the context of VDOM.

So, anyway, do you mean this?

patch(node: Element, newVNode: VNode): Element

[jorgebucaran]

@estrattonbailey Thanks! I left you a comment there. 🎉

[mindplay-dk]

I think this kinda comes back to the issue I mentioned in my last comment here - the assumption with patch() is that you always want to render a single VNode into a single parent element.

In my own experimental VDOM implementation, I used a signature like:

updateElements(parent: Element, oldNodes: VNodeList, newNodes: VNodeList)

That is, the update operation is with a single parent but a range of VNodes.

The "single parent to single child" relationship assumption made by patch means that, currently, I can only implement support for class-based components by using a dummy parent element (for example a div) to hook into the life-cycle via oncreate/onupdate.

I'm honestly starting to wonder if element-level life-cycle events can allow for class-based components at all? Take this trivial example:

<div>
  <Counter value={5} />
</div>

The problem here is that Counter does not directly map to an element at all - it's more like a placeholder or location marker for something else to be rendered, which means there is no element with life-cycle events for you to hook into.

I'm not even sure if "single parent / multiple children" addresses that - it probably doesn't?

It's making my head spin.

I'm close to giving up and just using Preact 🙄 I don't like the React component model at all, but it does work, but I'm sort of running out of steam - and I have real projects I need to build...

[jorgebucaran]

@mindplay-dk And you want stateful components?

[estrattonbailey]

@mindplay-dk yep! I think we're trying to solve the same puzzle.

As you mentioned here:

The problem is, patch only lets you patch the entire contents of an entire element.

Prior to the proposal of #91 which introduces private references to the actual DOM nodes via node._ref, picodom based its patch on the root DOM node that the app is mounted to. If every DOM node has a pointer that relates it to a real node in the DOM, patch can be used to morph a subtree of vdom nodes.

And yep, as you mention here, creating a callable mount proxy at an application level does work, and could likely be used for stateful components as well i.e. update local state, and then call the top-level application render. This isn't ideal, since picodom then needs to walk the entire tree instead of just the subtree of the stateful component.

I put together a stateful component POC here to demonstrate #91. I haven't tried to figure out an implementation as a class, but it might be doable.

[JSteunou]

@jorgebucaran yes I did mean

patch(node: Element, newVNode: VNode): Element

@mindplay-dk I really like your idea. I know being forced to start any React component with a single root node was not ideal for some people. In any case the main idea is to avoid useless Element, or 2 singles Element in a row (single parent, single child, then true content)

Do you think it will possible to write a patch function with this signature?

patch(nodes: Array<Element>, newVNodes: Array<VNode>): Array<Element>

maybe getting parent from 1st Element when existing or old Elements are given

[jorgebucaran]

@JSteunou patch(node: Element, newVNode: VNode): Element where is the oldNode, how is patch going to work without it?

[mindplay-dk]

Do you think it will possible to write a patch function with this signature?

Sure, but I wouldn't - you'd have to diff VNodes against DOM elements, which is going to be a lot more complex (and likely slower) than diffing the three properties of the simple VNode model.

Though maybe there's a way around that. What if you attached oldProps to the DOM element? This would make diffing just as simple and easy.

I think I'd swap the order of the arguments then.

patch(node: VNode, element?: Element): Element & VNodeState

That is, with the element being optional, and the returned Element being decorated with the updated VNode state, so we can diff during the next call.

Yeah, could work?! Requires a rewrite basically, but you've been wanting to do that anyway ;-)

[JSteunou]

@jorgebucaran I dont know where it is :) ATM I'm just thinking about the API for the user. I really like to think API 1st and hide the complexity to the user under the hood ;)

@mindplay-dk Yep, reversing the arguments might be better if the Element can be optional for the 1st patch call.

[jorgebucaran]

@JSteunou I really like to think API 1st and hide the complexity to the user under the hood ;)

I agree, but I also don't recommend using Picodom to make SPAs without a state management or framework layer. That's not to say I don't want to improve the API, but the goal of this project is for users to make their own custom Hyperapp-like view library/framework.

[JSteunou]

Yep, I was thinking API users, not necessary final user on top of the FW on top of picodom. Even for those it should be as easy as pie

[mindplay-dk]

IMO we should not prioritize easy over simple and correct - this is especially bad in APIs, where making something easier often means it's less flexible and unable to support certain uses.

If somebody (me!) wants to use vanilla Picodom, that's "my problem" - and I could easily change my mind about certain things in my own architecture from project to project, so I'd strongly prefer the API isn't optimized for any imagined use-case or opinions about architecture. It's a library: it should do just exactly what it needs to do, and whatever that means for the API is fine by me.

[mindplay-dk]

(this is merely a philosophical rant, I'm not disagreeing with anything that was said 😆)

[mindplay-dk]

I was thinking about the idea of attaching the entire VNode to the actual DOM element - and I'm pretty sure this would solve the problem with deferred onremove causing reordering.

When we create elements, adding the VNode property will identify them as elements we created - when we iterate over elements, simply ignore any elements that don't have the VNode property, so the property doubles as a flag that tells us which elements we're managing, as well as maintaining state between calls.

When a removed element is found, if it's removal is deferred, just remove it's VNode property, and it'll stay in the DOM wherever it is, since we won't even consider it as one of our managed elements.

Also, when we attach the VNode, we can also attach it's child index in the list of VNode children - which removes the need to count of consider element indexes or worry about changes in the DOM at all. This will even make it possible to inject custom siblings via oncreate, since DOM order doesn't affect us. Deferred element destruction won't cause problems either.

I'm pretty sure this approach will actually be even simpler than what we're doing now, too 😄

Man, I wish I had time to work on this right now...

[jorgebucaran]

@mindplay-dk I'm pretty sure this would solve the problem with deferred onremove causing reordering.

This is good.

[JSteunou]

@mindplay-dk HyperHTML does attach the Array from template string to the node Element I think

[mindplay-dk]

Attaching anything to the DOM is perhaps not ideal, but it would work as proof of concept - if needed, we could then use a jQuery-style data-strategy, where we assign a unique ID to an attribute of DOM elements, keep the actual data in an internal map, and remove garbage entries when we remove the DOM elements.

[mindplay-dk]

I've started now three times and ended up breaking everything, so now I'm trying to do this in very small increments - here's the first little step towards maintaining current VDom state in a vDom property in the actual DOM elements:

master...mindplay-dk:vdom-context

It's only breaking three tests, but I'm out of time - please feel free to take a stab at this :-)

(for now it's ignoring the oldNode argument to patch, but I want to get patchElement internally working before I change the API and break all the tests - I need working tests, so I can verify the internal implementation one step at a time...)

[mindplay-dk]

After several more attempts, I think I've reached some important conclusions.

My biggest a-ha so far, is I think I've worked out the right signature of the inner patch function, which I'm now calling patchTarget, as it doesn't just patch elements, but any target node, including text nodes.

function patchTarget(
    target: Node | undefined,
    parent: Element,
    nextSibling: Node | undefined,
    node: VNode | string,
    callbacks: Function[]): Node

I know, that looks like a lot of arguments, but let me explain:

The function will be called for every visited Node, whether text nodes or elements, and whether they exist or not - when called with an undefined target, it will create the target, and the target is always returned, whether it was new or pre-existing.

In order to create and position all nodes, the function needs both the parent and nextSibling references - there is always a parent, but there may not be a nextSibling, if this is the last node in a range of children.

The function will completely update the state of the target node, including it's position in the DOM, which will be consistently set using e.g. parent.insertBefore(target, nextSibling) - the loop over VNode children will simply determine the recursive target and nextSibling arguments, and won't need to deal with position or setting any other DOM element state.

I'm passing in the list of callbacks to avoid problems with a global/shared list of callbacks later on - the patch function will create the empty list of callbacks and pass it. This is to ensure that nested calls to patch have separate callback lists, e.g. when an onupdate or oncreate handler generates a call to patch. (This may be an unidentified bug in the current implementation.)

Note that child iteration needs to be in backwards order (last child first) because we use the nextSibling reference to create new elements, and some child targets may be undefined when we enter the loop.

Here's pseudo-code of the inner function:

    // if no target {
    //     create target
    // } else if target incompatible {
    //     destroy target
    //     create new target
    // }
    // 
    // if target.nextSibling != nextSibling {
    //     parent.insertBefore(target, nextSibling)
    // }
    //
    // if target is text node {
    //     update text content
    // } else {
    //     update target props and attributes
    //
    //     create list of managed children
    //     create a map of keyed children
    //    
    //     for each child in backwards order {
    //         if child has been flagged as removed {
    //             continue
    //         }
    //         if child has key and element already exists {
    //             recursively patch existing element
    //         } else {
    //             recursively patch undefined element
    //         }
    //         put returned child back into list of managed children
    //     }
    //     
    //     destroy any remaining unused keyed children
    // }
    //
    // return target

The "list of managed children" is built by looping over DOM elements and checking for the injected _vNode property, which contains the last known state.

To "destroy" here means removing the target element from active life-cycle management - actual element destruction is immediate only if there is no deferred removal hook, same as now, but we'll flag elements as removed, e.g. by injecting a _removed property, and these elements will still appear in the list of managed children, but will be ignored in the inner loop.

Because oldNode is no longer passed to us, now we decide what's in the list of old nodes, which will be any managed node with a _vNode property, including those that have been flagged as _removed - this eliminates the unexpected reordering problem, as these removed nodes are still considered valid as far as positioning, e.g. are still used as nextSibling references in the child loop.

I'm fairly sure about this by now, but it's basically a rewrite, and it'll break all the tests, so this is basically a start over from scratch situation.

I want to use Typescript and tape for the tests, which I tried on my first attempt - the big plus is tests turn around in a second, compared with 5-10 seconds with jest. I don't know why it's is so slow? But tape is much simpler and has everything we need. My first implementation was a single function - in my next attempt, I'll use lots of functions like in current picodom, and lots of inline documentation.

Rewriting this will take at least a few full days, and I don't know when I'll have that kind of time, which is why I'm dumping the explanation and pseudo-code here for now, in case somebody else wants to take a stab at an implementation 🙄

[dancespiele]

@mindplay-dk would your proposal break this?

import { State, View} from './helpers';
import { patch } from 'picodom';

let node: any;

export function render(view: View, state: State) {
    patch(node, (node = view(state)));
}

It is right that I would like to have some possibility when I use router and change the path that the parent element runs oncreate instead of onupdate but also right that I wouldn't like to change this algorism:

private build(configRouters: Array<Routers>, parentPath?: string) {
        configRouters.forEach((router, index) => {
            if(parentPath) router.path = `${parentPath}${router.path}`;
            if(router.alias) {
                this.router.on(router.path, {
                    as: router.alias, uses: (params, query) => {
                        this.setPatch(router, params, query);
                    }
                }, router.hooks);
            } else {

                this.router.on(router.path, (params, query) => {
                    this.setPatch(router, params, query);
                }, router.hooks);
            }
            if(router.routers) this.build(router.routers, router.path);
        });
    }

private setPatch(route: Routers, params: Object, query: string) {
        const page = route.page;
        const state: State = {};
        Object.assign(state, page.state);
        state.params = params;
        state.query = query;
        state.defaultProps = route.defaultProps || this.defaultProps;
        render(page.view, state);
}

[mindplay-dk]

@dancespiele this change will make it very easy to offer a meaningful mount function, which is basically what you're doing here. If anything, what you're doing will likely be simpler :-)

[estrattonbailey]

I've abandoned my original plans since proposing #91, but I've stayed subscribed to this thread because I find it interesting, lol.

On that note, my goal there was twofold:

1. Create a "meaningful" mount function
2. Create a simpler patch function (which would satisfy the original problem this issue was opened to address)

As you can see in here, patch accepts two VDOM nodes, and uses internal _ref props that associate mounted nodes to their DOM node counterparts to find parent, children, and nextSibling references.

JorgeBucaran expressed some concern with mutating the node structure with private fields. I don't share that concern, but I am probably inexperienced in this area so I defer.

The same pattern could definitely be achieved using @mindplay-dk's proposal of an internal map. I had considered this when I was poking around #91, but was concerned about reliably creating unique indices to associate VDOM nodes to DOM nodes in the map, as well as the additional lookup and memory allocation. Attaching it to the VDOM node via _ref seemed like a more stable and compact solution.

At any rate, my 2¢ is that there should be a separate mount function that initializes a VDOM tree. Then there should be a patch function solely for patching said VDOM tree.

It's been fun to see this discussion unfold :)

[mindplay-dk]

JorgeBucaran expressed some concern with mutating the node structure with private fields

What I'm proposing is the opposite: inject the vNode into the DOM element.

This makes the DOM the "source of truth", and removes the need to pass in / maintain the old vNode state yourself, allows for deferred removal, obviates the reordering issue - it even allows you to manually move/destroy managed DOM nodes, for example during drag-and-drop operations, and a simple call to update your mounted node afterwards will put everything back in working order.

And as said, there's a way around injecting vDom nodes into DOM elements, too - by using a reserved DOM attribute to store a global identifier for an element registry, a'la .data() in jQuery. First things first though.

At any rate, my 2¢ is that there should be a separate mount function that initializes a VDOM tree

Absolutely, but it's not really possible right now, since you can't designate an update of single, existing element - because, as you pointed out in your first comment, parent.children[0] is the root of every update, which means that every update must have a dedicated parent.

What I'm proposing mitigates this, but almost necessitates the addition of a mount function, since the constraint is you must have a pre-existing element to patch (as it provides the parent reference and child index) or accept that the newly created element will end up getting inserted into the end of the list.

[mindplay-dk]

I'm going to work on this today.

I have trouble figuring out what the top-level patch function should look like, or whether patch is even meaningful as the top-level function?

The problem is the difference between creating new elements vs patching existing elements - the old patch solves that by always creating an immediate child, which creates the direct parent/child relationship problem we discussed above.

The new function should patch an individual element directly, replace it as needed, etc. - I can't think of a meaningful way to do that with a single function, since the required input for creation and update are substantially different, e.g. minimally:

• create(parent, nextSibling, vNode): Element
• update(element, vNode)

Because creation is more or less meaningless if you can't specify where in the DOM you want things created.

I will go ahead and work on the internal function, keeping the exported function and the current parent/child constraint still in place, but I'm really looking for ideas and input here...

[mindplay-dk]

Alright, guys - here's the fruit of today's labor:

https://bitbucket.org/mindplaydk/veedom/src

It's a rough implementation of the pseudo-code I posted above.

I based off of my own "Picodom rewrite", as this had tests and a working h function already written in Typescript. Compared to my first attempt, the current state of this is much closer to Picodom in structure, e.g. similarly factored in terms of internal functions, but fully type-hinted.

My aim right now is to make it work: I am putting zero effort into keeping the code small or free of duplication, it's deliberately very verbose, unoptimized, and commented.

This will need at least another full day of work to fix bugs and optimize - it isn't passing the tests yet, and I'm out of time for today, but it feels like a pretty good start :-)

[mindplay-dk]

Added a quick build script displaying the minified/gzipped output size, and a bit to my surprise, it's only 1172 bytes, having made no attempts to remove duplication or make it smaller 😃

Anyways, that's all I've got time for today.

[mindplay-dk]

To be fair, it still doesn't have style support, but it does have object/array class support, so likely this'll end up somewhere in the same ~1K region when it's all said and done.

[dancespiele]

@mindplay-dk the advertising for view library is 1k if it is more this would be a misleading advertising ;)

[mindplay-dk]

I've said that before, but my personal goal is to have something that is simple and useful - I'm only counting bytes to get an idea of whether it's still small, the impact of making a change, and so on. 1K or 2K is besides the point for me :-)

[dancespiele]

I was kidding :). I would like collaborate but I will be busy the next two weeks, after that I will test everything and also try to give some idea. @mindplay-dk good job.

[mindplay-dk]

good job

if it actually works ;-)

[mindplay-dk]

Sorry, guys, but I'm going to drop this rewrite for now - I simply can't dedicate the time and attention this needs, and it's a constant distraction. Being able to work on this for a day or so every two weeks is a futile effort - I can't recall what I was doing, and basically have to start over every time.

I'm hoping somebody else will pick up where I left off - the idea of the inner function patchTarget, which is responsible for creating/updating/positioning every node, is pretty solid, I think. The codebase is fully type-hinted Typescript in strict mode, and the test-suite is much simpler and faster, and also fully type-hinted, so I think this makes a really solid starting point.

If somebody wants to pick up work on this, please note that the inner loop over child VNodes is completely unfinished - it of course will need two index variables, I somehow thought this would be possible with just one, but that part of the pseudo-code was totally naive and unfinished. I had a working implementation earlier here, but I can't figure out how to merge these concepts.

I'd be happy to answer any questions.

[jorgebucaran]

This is it, folks!

import { h, patch } from "patchdom"

document.body.appendChild(
  patch( h("h1", {}, "Hello") )
)

The patch function now returns an element, if none is given, allowing you to append it to a container if that's what you want. If you pass an existing element instead, patchdom will patch that element (attempting to recycle any existing markup), instead of using it as a container like in the past.

[JSteunou]

👏 👏 that was exactly the idea ! No need for a wontfix label, this is fixed ! :)

[jorgebucaran]

🎉🤘😄🤘🎉

[jorgebucaran]

I am writing docs now.