render function mutating host property breaks rendering in v8.1.6

[Hyzual]

Hello,

I was about to update to v8.1.8 but when testing I've found an issue. I'm not sure if the issue is in the cache of properties or the HTML rendering.

A bit of context: we want to call an old library that queries the DOM and creates tool-tips that appear when hovering certain kinds of elements. We need to call if after rendering (because it queries the DOM) and we want to call it only once, to avoid creating several times the same tool-tips. To do so, we have written a "render factory": a "middleware" function that takes an UpdateFunction in parameter, calls it with host, then calls our tool-tip library, then returns the result of the UpdateFunction. It worked well but updating to v8.1.6 (and later) breaks our button's behaviour. So I'm wondering: are we doing it wrong (and this change is expected), or is it a legit use of hybrids (and this change is a bug) ?

I've set up a reproduction here: https://codesandbox.io/s/hybrids-render-function-mutating-host-property-breaks-rendering-hu8dgm
In src/index.ts, if you don't change anything, when you click on the button, you should see an "on click" console log, but the reply is not rendered (nothing changes in the HTML). If you remove or comment the if() block in renderFactory at L21-24, when you click on the button, the reply should show up as expected. If you change the hybrids version to v8.1.5, clicking the button works as expected without any change in the code.

I have tried debugging what happens if you leave the if() block. It seems that the reply entry has an empty context property in the cache (which was not empty in v8.1.5), so nothing happens, the template is not rendered (even though the onClick is correctly triggered).

Again, I'm not sure whether we are doing something wrong or whether it's a bug. Please let met know if I can help further. Have a nice day !

[smalluban]

Again, thanks for submitting the issue. In 8.1.6 I made a major refactor of the core mechanisms, however, they suppose to be not break public API, so it was a patch release.

The main reason behind it was the #195 (comment). Additionally, I made some performance improvements.

I will look at your example, and try to find out what is going on. It's kind of late in my timezone, so probably tomorrow I'll have answers.

[Hyzual]

Thanks a lot, don't worry we are not in a rush to update! Take all the time needed!

[smalluban]

Generally, what you are trying to do should be avoided. I mean setting property value within getting another property, which depends on that value. For easier understanding:

GET A -> GET B, SET B

The cache saves "B" as a deps of "A", but within getting the "A", you set the "B". In earlier versions of the hybrids, it would throw an error. Later, when introducing the store feature, I had to do it in some special places (but it was under the control of the internals).

In that terms, the cache refactor in 8.1.6 was still valid with the API. It can be "fixed" (but still, it is not strictly a bug), so I've created PR #203, which makes it work more like before. The codesandbox should create a package with a change, so you can try it out.

However, I encourage you to change the logic and avoid setting values while you get them. This is why there is an observe method. I know, that it is not available for the render and content properties, but you can create a separate property, which can depend on it, and have the same result, without a circular problem:

{
  have_tooltips_been_loaded: false,
  sideEffects: {
    get: ({ content }) => content(),
    observe(host) => {
      host.have_tooltips_been_loaded = true;
      // other side effects
    },
   content: (...) => ...,
  },
};

The sideEffects property will trigger observe method only once, as it always returns the host, but calls content(), so it always runs after the render.

I know that it is also a kind of a "hack", so I will think about a cleaner solutions for connect and observe methods for content and render properties.

EDIT: The codesandbox is better than I thought - they already built your code example for the PR, and it looks that it works now :)

[Hyzual]

Thanks a lot for your detailed answer ! I agree that it's not a bug, I've changed our logic. Indeed it is cleaner to have a dedicated sideEffects property, and it triggers only once. We've made the change and updating to v8.1.8 works 🎉.

I have also a separate case where we want to trigger some code every time after the rendering, because we need to get the height of the rendered element to affect another element. We were using setTimeout() to (hopefully) wait after the rendering. Your example gave me the idea to use a property and observe to do it better (and also leverage the cache), and it worked 🎉

type MyElement = {
  readonly element_height: number;
  readonly post_rendering_callback: (() => void) | undefined;
  readonly content: () => HTMLElement;
}

define<MyElement>({
  tag: "my-element";
  element_height: {
    get: (host) => host.content().getBoundingClientRect().height,
    observe(host) {
      host.post_rendering_callback?.();
    },
  },
  post_rendering_callback: undefined,
  content: (host) => html`...`
});

I'll close this issue as our problem is now solved. Thanks a lot for your time and efforts!

[smalluban]

It is a similar case to the original one, but this time the sideEffects property should return not the result of the call to content, but the property itself. The content property changes every time the re-render is needed (so it's built-in observe method runs).

{
  sideEffects: {
    get: ({ content }) => content,
    observe: (host) => {
       // side-effects here
    },
  },
}