-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug: Template is not updated when component properties change #229
Comments
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. |
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. |
I my not have time in upcoming days to look at this, but I'll definitely do, sorry for inconvenience. |
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 Because of your design of the components, the following actions are made, when you click for the first time on the date picker:
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 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 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 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 -> |
Good example @smalluban. Two questions.
I may still be at a loss as to how I can do this better. |
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 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:
|
@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. |
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.
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).
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. |
@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). |
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. |
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. |
@Qsppl I think the issue is fixed/explained. Feel free to re-open if you encounter any problems when refactoring your components. |
The <datetime-picker> 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 <datetime-picker> is clicked, it generates an 'input' event.
This event catches the <datetime-input> element and updates its "value" property.
When <datetime-input> receives a new value for the "value" property, it is closed (the "open" property is set to "false").
When the "open" property of the <datetime-input> component changes, the "open" property of the <datetime-picker> component takes on the same value.
Now <datetime-picker> has a new value for the "open" property, but it will not update the template.
The text was updated successfully, but these errors were encountered: