leaky proxy nodes when morphing #65

jongacnik · 2017-10-21T23:58:49Z

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:

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.

The text was updated successfully, but these errors were encountered:

yoshuawuyts · 2017-10-22T02:09:49Z

Gut feeling tells me it's related to choojs/nanomorph#85 and choojs/nanomorph#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 ***@***.***> 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 <#65>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACWlem7yVyVo2gZriZ9Avbpmj-Zn9tl6ks5suoU6gaJpZM4QBwi_> .

bcomnes · 2017-11-13T18:19:51Z

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.

s3ththompson · 2017-12-31T21:03:58Z

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)?

bcomnes · 2017-12-31T21:37:41Z

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.

bcomnes · 2018-01-01T02:31:27Z

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)

s3ththompson · 2018-06-30T15:13:32Z

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

bcomnes · 2018-06-30T19:14:49Z

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

s3ththompson · 2018-06-30T20:43:11Z

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

bennlich · 2023-12-04T12:20:38Z

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 #65 (comment) was merged (see 0e02a3a and choojs/nanomorph@985cdc3).

In @jongacnik 's example #65 (comment), 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 .

bcomnes · 2023-12-04T15:10:00Z

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. ✌️

bennlich · 2023-12-04T15:55:46Z

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!

bennlich · 2023-12-07T13:46:55Z

FWIW I implemented fix number 1 from #65 (comment) and that seems to work well. See: bennlich/nanomorph@2fee534

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).

bcomnes · 2023-12-07T15:28:38Z

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.

bcomnes added the help wanted label Jan 16, 2018

bcomnes mentioned this issue Feb 12, 2018

Discussion: choo/component choojs/choo#593

Closed

kristoferjoseph added a commit that referenced this issue Mar 2, 2018

Fixes #65 after nanomorph PR #98 lands

fb0cd9c

bennlich added a commit to bennlich/nanomorph that referenced this issue Dec 7, 2023

fix leaked proxies; see choojs/nanocomponent#65

2fee534

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

leaky proxy nodes when morphing #65

leaky proxy nodes when morphing #65

jongacnik commented Oct 21, 2017 •

edited

Loading

yoshuawuyts commented Oct 22, 2017 via email

bcomnes commented Nov 13, 2017

s3ththompson commented Dec 31, 2017

bcomnes commented Dec 31, 2017 •

edited

Loading

bcomnes commented Jan 1, 2018 •

edited

Loading

s3ththompson commented Jun 30, 2018

bcomnes commented Jun 30, 2018

s3ththompson commented Jun 30, 2018

bennlich commented Dec 4, 2023 •

edited

Loading

bcomnes commented Dec 4, 2023 •

edited

Loading

bennlich commented Dec 4, 2023

bennlich commented Dec 7, 2023

bcomnes commented Dec 7, 2023 •

edited

Loading

leaky proxy nodes when morphing #65

leaky proxy nodes when morphing #65

Comments

jongacnik commented Oct 21, 2017 • edited Loading

yoshuawuyts commented Oct 22, 2017 via email

bcomnes commented Nov 13, 2017

s3ththompson commented Dec 31, 2017

bcomnes commented Dec 31, 2017 • edited Loading

bcomnes commented Jan 1, 2018 • edited Loading

s3ththompson commented Jun 30, 2018

bcomnes commented Jun 30, 2018

s3ththompson commented Jun 30, 2018

bennlich commented Dec 4, 2023 • edited Loading

bcomnes commented Dec 4, 2023 • edited Loading

bennlich commented Dec 4, 2023

bennlich commented Dec 7, 2023

bcomnes commented Dec 7, 2023 • edited Loading

jongacnik commented Oct 21, 2017 •

edited

Loading

bcomnes commented Dec 31, 2017 •

edited

Loading

bcomnes commented Jan 1, 2018 •

edited

Loading

bennlich commented Dec 4, 2023 •

edited

Loading

bcomnes commented Dec 4, 2023 •

edited

Loading

bcomnes commented Dec 7, 2023 •

edited

Loading