Bug: Template is not updated when component properties change

[Qsppl]

The \ component has an "open" property.

It is equal to "true".

I set this property to "false".

The value was assigned, but the template was not updated.

This bug is difficult to reproduce, so I am attaching the code from my project.

How to reproduce the bug:

code: https://codepen.io/qsppl/pen/VwRLxPb?editors=1010

Just click on the button:

expected behavior: datepicker closes after time selection

observed behavior: datepicker did not close after selecting time

Explanation of the problem:

When one of the buttons in the \ is clicked, it generates an 'input' event. This event catches the \ element and updates its "value" property.

When \ receives a new value for the "value" property, it is closed (the "open" property is set to "false"). When the "open" property of the \ component changes, the "open" property of the \ component takes on the same value.

Now \ has a new value for the "open" property, but it will not update the template.

[Qsppl]

update: After the bug, the template is no longer updated at all. Try clicking on other buttons - the event listeners fire, and so do the component properties. But the template update function is no longer called for any changes.

[dobexx]

I have already noticed this behaviour in many of our hundreds of hybridsjs components. The only thing that helps here is a programmatic call to host.render(), which restarts the refresh.

Only with the introduction of the update from 5.x to the current 8.x version does this occur more frequently.

[dobexx]

So far I have not been able to reproduce it to open a ticket. Thanks @Qsppl for that.

Possibly one more hint. We use a lot of polymer components and we often use "onclick" events where this occurs frequently.

I only noticed this with the update to 8.x.

[smalluban]

I my not have time in upcoming days to look at this, but I'll definitely do, sorry for inconvenience.

[smalluban]

I debugged what's going on in your code example, and it looks that the root problem is updating itself properties during the "update" process.

The update process in hybrids is global (named "emitter"), which saves all of the functions (observers) that must be called in the next microtask (using Promise.resolve().then...). If the function is already there, during the update process (a loop, which calls that functions one by one), adding it to the Set won't call it again. This is not only for the endless loop protection but for the convenience, where you change your inputs (writeable properties) outside of that process, which one of those changes flags the emitter to start that process.

Generally, the observe() method is for side effects (as the docs says) - changing things outside of the scope of the component (external things) - not internal properties, which can be "in update" state.

Because of your design of the components, the following actions are made, when you click for the first time on the date picker:

1. focusin changes - so the cache checks, and adds focusin observer, and "render" observer to the set (the render depends on it)
2. Then the value is changed, so the value observer is added, which updates the open - but the render observer is before the value observer - it won't be called after the host.open is set in the observer

I can find many observers in your components, which I use very rarely (the built-in render is usually sufficient), and I am using hybrids in complex projects. You should avoid setting your own properties in some kind of useEffect from React-like frameworks. Instead, use getters to get the current value of the property, which depends on another value.

Usually, if something is hard to do in hybrids, try another way - it will be much better. I think that in your case, you needed the lastValue of the host.value - you check if it is undefined, so you have a "change". Instead, I would just set the host.open in the right place - in the side effect function attached to the input event. Even though, then you can't use html.set helper directly, the cost of that function is minimal:

function setValue(host, event) {
  const { value } = event.target;

  host.value = value;
  host.open = false;
}

I tried to fix your example, but you have so many two-way connections, that it's super hard to make it work ;) For example, your root component set's value, which triggers oninput event on the children, which goes back to parent and set host.open - don't do that...

However, I made a "broken" simplest example here: https://stackblitz.com/edit/hybrids-broken-update-process?file=src%2Findex.html,src%2Findex.js

function increaseCount(host) {
  host.count += 1;
}

define({
  tag: 'my-component',
  count: {
    value: 0,
    observe(host, value) {
      host.other = host.count;
    },
  },
  other: {
    value: 0,
    observe(host, value) {
      host.prop = value;
    },
  },
  prop: 0,
  render: ({ count, other, prop }) => html`
    <p>Count: ${count}</p>
    <p>Other: ${other}</p>
    <p>Property updated in observer: ${prop}</p>

    <button onclick="${increaseCount}">
      Increase count
    </button>
  `,
});

In the above example there is a chain of updates -> count -> other -> prop, as the render is already a dep of the count property, it is added to the queue when count updates, then other observers triggers, to the prop is updated, but already after the render was called.

[dobexx]

Good example @smalluban.

Two questions.

1. in our case we had version 5.x of hybridsjs for a very long time and there were no such situations. Was this because the "observer/emitter" were more generous here and liked to perform a "render" once more?

2. In your example, clicking on "increase Count" still increases the attributes and properties of the component, but does not update the template. Why does a host.render() have to be explicitly called here so that the template is updated again?

I may still be at a loss as to how I can do this better.

[smalluban]

Ad 1. Since 5.x there were a lot of changes (usually simplification) of the cache/emitter implementation, so before, it might have been the case, that the order or timing of calling observers was different - but still - if you follow the one way (set inputs outside of the update process, and then update) everything should work as before.

Ad 2. The render is called once, and "should be" twice, as the value, which renders depends on changes after the render is called (but within the update process). That's why calling it manually "fixes" the issue. Properties have always correct value, the problem is with calling render. However, if it would call render again, then we would have endless loops, and I suppose none of the modern frameworks support that case (but might be harder to make it inside of the render process).

This is the first time I figured out such a case with nested observers... It's hard to say what you should do without an example. However, I recommend:

1. Avoid using observers to set other properties from the same component, or related ones (which you pass props down)
2. Use only one-way control: parent -> child or child -> parent - if passing value down triggers an event, which sets something back on the parent, it must blow up eventually ;) (I think this is a general design rule, not only related to hybrids)

[smalluban]

@dobexx I found, that render deps are broken for good after that chain of observes (actually setters, which cause cache to invalidate). Fortunately, there is a solution, that protects from permanent breakage (but still it can't fix the issue with out-of-date one of the props in render).

Look at the linked PR. It's possible to test it out in the codesandbox auto-generated package. I know this tool is far from perfect (I prefer StackBlitz), but I think there is no other way to test it before release.

[Qsppl]

As far as I understand, I made two types of errors in the code and one in the design:

1. observe() should not change the properties of hybrids components.
2. You cannot pass state to a child component by changing the html attribute (<component attribute="value">). You need to work with BOM objects (component.property = value).
3. Component states need to be lifting to the parent.

Thanks, this solves the problem. I know about “lifting state up” to the parent from experience with React, but I thought that it was necessary due to the specifics of how “virtual-dom” works. I think we need to add a section on this topic to the documentation. And somehow generate a warning about this error in the console so that developers understand what they are doing wrong, because it is very difficult to understand what the problem is.

If you can't use observe(), there's a lot of extra code in factories [1]

function myCustomProperty(multiplier) {
  return {
    get: (host, value) => value | 0,
    set: (host, value) => {
      if (value > 100) return value + 3
      if (value >= 0) return value * multiplier
      if (value < 0) return value * 2 * multiplier
    },
  };
}

function myCustomProperty(multiplier, callbacks) {
  return {
    get: (host, value) => value | 0,
    set: async (host, value) => {
      if (value > 100) {
        await Promise.all(callbacks.map(calback(host, value + 3)))
        return value + 3
      }

      if (value >= 0) {
        await Promise.all(callbacks.map(calback(host, value * multiplier)))
        return value * multiplier
      }

      if (value < 0) {
        await Promise.all(callbacks.map(calback(host, value * 2 * multiplier)))
        return value * 2 * multiplier
      }
    },
  };
}

This code is harder to read, harder to keep track of, and harder to inspect. Is it possible to create an analogue of observe() designed to change the properties of components?

I can't lifting state up [2 & 3]

Also, I can't use the children() and parent() factories - they return elements generated by the content() function, but not by the render() function.

I can't avoid creating markup in the render() function because I need to isolate events, stylesheets and other functionality in shadow-dom. It follows from this that I need to leave slots in render() and fill them with elements generated in content().

But in reality this is not a universal solution. For example, when I need to wrap some of the markup from render() into a hybrids component <expand-collapse> - I can't use the slot. The content of a slot is "default slot content" - you cannot wrap a piece of content into a slot - that content will be replaced by the element you insert into the slot.

[smalluban]

1. observe() should not change the properties of hybrids components.

It depends.. and that's why it is not blocked (it does not throw). You must create a very nested observe pattern, to break rendering (like my example - it must be two nested observers). However, yes - avoid that, as this is a stateful action, which depends on the previous and current state of the property - it is always better to create get/set pure methods, which always return the same value, and depend only on arguments.

2. You cannot pass state to a child component by changing the html attribute (). You need to work with BOM objects (component.property = value).

This I am not sure if I understand. Hybrids template engine automatically chooses a property or attribute when setting. It works well with both. The hybrids properties don't react to attribute change, but this is another thing (which is fully correct).

3. Component states need to be lifted to the parent.

Hybrids supports both up and down state passing, but you should never do both at once - this is a problem with your calendar component - value is in parent and children, and changing it causes a loop of changes.

[smalluban]

@dobexx @Qsppl I've released v8.2.10 with a fix, which should protect from the broken state of the render after nested observers, etc.. It should at least protect from a case, that the render is no more updating ;)

@Qsppl Your original example from the first comment with v8.2.10 works much better - the calendar does not break (still I think it should hide, but it doesn't - but I explained why).

[Qsppl]

Thank you.

In the near future I will write a new version of the component in accordance with the fixes you specified to make sure that it will work properly.

If everything is fine, I will close the issue.

[dobexx]

Brilliant @smalluban, now it works perfectly with the updated version. We were able to remove all "host.render()" calls. We had already paid close attention to the direction of the data update. We come from the world of Polymer.js and know the debugging scenarios. Once again, great work.

[smalluban]

@Qsppl I think the issue is fixed/explained. Feel free to re-open if you encounter any problems when refactoring your components.