nanocomponent leaky proxy nodes when morphing

Not sure if we want this issue here or in nanomorph, but looks like proxy nodes are leaked when a component is morphed to a new location in the dom. For example, if these two views are morphed, a proxy node will be rendered rather than the component:

function viewA (state, emit) {
  return html`
    <body>
      <a href="/">beep</a>
      <a href="/boop">boop</a>
      <div>
        ${component.render()}
      </div>
    </body>
  `
}

function viewB (state, emit) {
  return html`
    <body>
      <a href="/">beep</a>
      <a href="/boop">boop</a>
      ${component.render()}
    </body>
  `
}

You can see this behavior here:

Demo: https://choo-leaky-proxy.glitch.me
Code: https://glitch.com/edit/#!/choo-leaky-proxy?path=index.js:1:0

Also interesting to note if the component's update function returns true, the component will leak the proxy node on first morph, while subsequent morphs will correctly return the component.

Oct 21 '17 23:10 jongacnik

Gut feeling tells me it's related to https://github.com/choojs/nanomorph/issues/85 and https://github.com/choojs/nanomorph/issues/86. Something is off; and having a better element reorder algo (e.g. one that's designed, rather than hacked together) might help us solve the issues.

On Sun, Oct 22, 2017 at 1:58 AM Jon Gacnik [email protected] wrote:

Not sure if we want this issue here or in nanomorph https://github.com/choojs/nanomorph, but looks like proxy nodes are leaked when a component is morphed to a new location in the dom. For example, if these two views are morphed, a proxy node will be rendered rather than the component:

function viewA (state, emit) { return html <body> <a href="/">beep</a> <a href="/boop">boop</a> <div> ${component.render()} </div> </body> } function viewB (state, emit) { return html <body> <a href="/">beep</a> <a href="/boop">boop</a> ${component.render()} </body> }

You can see this behavior here:

Demo: https://choo-leaky-proxy.glitch.me

Code: https://glitch.com/edit/#!/choo-leaky-proxy?path=index.js:1:0

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/choojs/nanocomponent/issues/65, or mute the thread https://github.com/notifications/unsubscribe-auth/ACWlem7yVyVo2gZriZ9Avbpmj-Zn9tl6ks5suoU6gaJpZM4QBwi_ .

Oct 22 '17 02:10 yoshuawuyts

Work around for now is to create two instances, one for each view/dom location. Nanomorph/component is smart enough to still efficiently morph between the two, but you will incur the cost of rendering when switching views.

Nov 13 '17 18:11 bcomnes

Aha! Ran into this one on the last app I was working on--I ended up getting rid of a component in the header to fix. Nice to see that this was likely the root issue.

@yoshuawuyts what's your take on next steps for a better diffing algo? is the blocker on the implementation side (finding someone who has time to implement and test a new algo) or on the design side (figuring out what the correct tradeoffs / optimizations are for an algo which operates on real DOM nodes, unlike vdom algos)?

Dec 31 '17 21:12 s3ththompson

I think we could add a fairly easy work around into nanomorph.

The problem occurs when you create a diffTree with proxyNodes, and during the morph, the realNode that the proxyNode points to is removed prior to the proxyNode getting morphed into the realDom. Child re-ordering can typically handle a lot of these situations, but if the proxyNode is pointing to a realNode that isn't in the realTree or apart of its sibling set of the realTree, nanomorph leaks proxyNodes.

Two ideas:

When nanomorph receives a diffTree, querySelectAll a standard proxy identifier prior to morphing and use that to store references to the realNodes that the proxyNode is pointing at. In the event nanomorph wants to replace a realTree node with a proxyNode, check if we have a reference to this realNode that was removed from the tree and use that instead. 👍
Have nanocomponent proxy nodes detect if they are ever put into the page and nanomorph the proxy back into whatever its supposed to be by storing a direct node reference every time the this.element getter is used. Would likely require on-load in all cases 👎

I think 1 is a more robust solution, but it could be a departure from the isSameNode idea we've been trying to stick to.

This would still be very fast.

The only remaining issue with nanomorph after we fix the leaking nodes is that child reordering has some worse case scenarios, e.g. where the entire sibling set morph cascades the entire set, rather than simple insertions and removals, which a diff set algorithm would fix. See http://nanomap.netlify.com for some examples of this.

Dec 31 '17 21:12 bcomnes

Third idea is to store a reference to the realNode in the proxyNode when its created so that it can be retrieved or returned from isSameNode or off of another property of the proxy.

e.g. if (isProxy(proxyNode) && !proxyNode.isSameNode(el)) replace(el, proxyNode.realNode)

Jan 01 '18 02:01 bcomnes

@bcomnes @kristoferjoseph is this issue closed? any update on nanocomponent beta being merged into master?

Jun 30 '18 15:06 s3ththompson

Last I head there were still issues with the beta approach. I haven't had time to dig in.

Jun 30 '18 19:06 bcomnes

@bcomnes any pointers to test cases or anecdotal evidence? i would be happy to dig in too!

Jun 30 '18 20:06 s3ththompson

5 years later, because I still love nanohtml / nanomorph / nanocomponent (and also because this bug is really annoying in practice)...

I just tested https://github.com/choojs/nanocomponent/tree/v7.0.0-beta1 and https://github.com/choojs/nanomorph/tree/v6.0.0-beta1, where the fix @bcomnes mentioned in https://github.com/choojs/nanocomponent/issues/65#issuecomment-354633695 was merged (see https://github.com/choojs/nanocomponent/commit/0e02a3a4e339baf8ba7a54e9279148c3c45407d9 and https://github.com/choojs/nanomorph/commit/985cdc39e8b9ec4bccf8af180c6510b3b96e0243).

In @jongacnik 's example https://github.com/choojs/nanocomponent/issues/65#issue-267423422, morphing from viewA to viewB is fixed (i.e. from <div>${component.render()}</div> -> ${component.render()}), but the proxy still leaks if you try to go from viewB to viewA (i.e. from ${component.render()} -> <div>${component.render()}</div>).

This is because, in the second case, the element that gets isProxy() tested is the plain old <div></div>. The leaky proxy is its child, so it still sneaks by undetected.

Maybe morph should do a walk of any node it is planning to insert, check for proxies anywhere in the node's tree, and then perform therealNode replacement before inserting? Curious what you think @bcomnes @s3ththompson .

Dec 04 '23 12:12 bennlich

Blast from the past. There were a lot of cool ideas here that were ahead of their time but I've since switched to running a hooks based approach. I've found https://github.com/WebReflection/uland to meet all of the same design goals I liked in choo, but also provide a more more modern hooks based api for components, which is a much more natural fit for the functional reactive component template rendering idea than classes, on top of being a more correct implementation of the dom diffing algorithm.

A sample example of a full stack application running uland inside a web framework I wrote called top-bun can be found here: https://github.com/hifiwi-fi/breadcrum.net/tree/master/web

In terms of nanocomponent, I don't really intend to develop this idea further and don't recommend others try to fix it either. There are still a few older projects relying on it and I've been uninvolved long enough to not know how to really best serve them through changes here. I suggest exploring new ideas by forking and experimenting under a new name. The good news is that choo always accommodated alternative component models very well. ✌️

Dec 04 '23 15:12 bcomnes

Cool! Thanks for this. I found uhtml through your blog just the other day and thought it looked pretty exciting, but didn't dive deep enough to find uland or understand how components would work in that framework. Definitely going to check it out!

Dec 04 '23 15:12 bennlich

FWIW I implemented fix number 1 from https://github.com/choojs/nanocomponent/issues/65#issuecomment-354624804 and that seems to work well. See: https://github.com/bennlich/nanomorph/commit/2fee5344347cb879b144f603928a03f3615e4de6

I may switch to uhtml in the future (a lot of the html template string features look delicious), but TBH I don't like hooks much at all (though I'm curious to see if I like signals any better). nanohtml + nanomorph + nanocomponent really hits a sweetspot for me in terms of simplicity (both in terms of their source, and in terms of the experience as a developer).

Dec 07 '23 13:12 bennlich

I didn't like them either at first. The no branching constraint seems weird. But then you use them and realize it's a much better fit for functional declarative rendering.

For example, if you want to share behavior between 2 or more components that spans multiple lifecycles that includes state, say, mount and unmount and a couple state variables. The class based approach requires that you implement that behavior across multiple methods. Worse, if you have 2 or more reusable behaviors, you end up mixing these all together in the same methods per component. With hooks, you can wrap state and lifecycles together in a single reusable function call. You then can just import them and compose them together at the top of the component. No more higher order components. No more this programming. Composable reusable state and effect code was so unwieldy in the class based approach, people basically didn't do it (think of all those jsx wrappers that acted as silent prop injection providers).

Not sure if that makes any sense until you try it but once you see what it allows, you'll be badumchh hooked.

Dec 07 '23 15:12 bcomnes

nanocomponent nanocomponent copied to clipboard

leaky proxy nodes when morphing

nanocomponent
nanocomponent copied to clipboard