Closed vogloblinsky closed 5 years ago
No. Pretty much everything is wrong there, starting from the fact you are mistaking HyperHTMLElement with hyperHTML, continuing with the fact you are not using HyperHTMLElement features, like addressing all arrow function and listeners instead of using the implicit handleEvent, passing through the fact you are invalidating items so that these are trashed each time.
In few words, whatever conclusion you'll write about hyperHTML will be misleading, and also pretty wrong, 'cause size, performance, and library strength is nullified by improper usage (it works anyway, which is a strength too, but not relevant for benchmarks).
I'm on vacation so I'm not sure I'll have time to PR all the needed changes but please remove hyperHTML from that benchmark if you are planning to write anything about hyperHTML through that test, thank you.
Oh keep cool, this issue was created to discuss with you, just to be sure i was not "mistaking" something. I doesn't want to make conclusions before being sure everything is in sync with hyperHTML philosophy. I will look at your remarks.
to clarify remarks:
Same you mentioned lit-element, and not lit-html, you should mention HyperHTMLElement, not hyperHTML, 'cause hyperHTML doesn't care, use, need, Shadow DOM, as example, and it's not directly related to Custom Elements.
If you need onmethod
handler, you define that on the class and then <div onmethod=${this}>
on the render. You never need to duplicate per each instance listeners, with hyperHTML, you use the native handleEvent pattern provided by default.
Everything else is unnecessary code, ram, and performance, overhead.
If you are down with immutability, you need to re-think wires too, 'cause otherwise you trash every item related node when you Object.assign({}, before, {checked: !checked})
, dropping the previously known reference.
Since TODOs have no reason to be immutable, being stateful by definition, you either mute the item when needed, or associate their unique value through an ID, so you preserve nodes and you have keyed performance, instead of trashed layout and GC.
With all these things done right, you'll surely have different results that better represent HyperHTMLElement.
Thanks for the details.
I have updated hyperhtml implementation : https://github.com/vogloblinsky/web-components-benchmark/blob/master/todomvc/hyperHTMLElement/js/my-todo.js
Score for edit todos are better, from 989ms to 102ms, others operations are the same.
Is there enhancements possible for pascal-triangle implementation ? https://github.com/vogloblinsky/web-components-benchmark/tree/master/pascal-triangle/hyperHTMLElement/js
when you setState
the render/update is done automatically, you are doubling every single render for no reason as it is now.
You should remove the this.render();
from every method except the created one.
for the pascal-triangle you could relate unique items:
// why don't you access `wire` once?
const {wire} = HyperHTMLElement;
// ... within the render ...
${
this._list.map((line, y) => wire(this, `:${y}`)`
<div>${
line.map((item, x) => wire(this, `:${y}-${x}`)`
<triangle-item text="${item}"/>`)
}</div>`)
}
Also note you can self-close tags in hyperHTML, making it more xHTML and JSX friendly.
P.S. I don't understand what's the value of having ShadowDOM in this class: https://github.com/vogloblinsky/web-components-benchmark/blob/master/pascal-triangle/hyperHTMLElement/js/triangle-item.js
looks redundant/overhead for no reason
when you
setState
the render/update is done automatically, you are doubling every single render for no reason as it is now.You should remove the
this.render();
from every method except the created one.
Sorry but just calling setState
doesn't recall this.render
right after.
I don't think so WebReflection/hyperHTML-Element:index.js@
master
#L2308-L2314 WebReflection/hyperHTML-Element:index.js@master
#L812-L819
Ok sorry, my bad, i had override internal setState
method following your README. Works fine now.
I don't think so, one more time ... https://github.com/WebReflection/hyperHTML-Element#the-class
I don't think so, one more time ... WebReflection/hyperHTML-Element#the-class
Sorry, i have edited last comment, it works fine now 😄
glad it does, yet my README never suggested to overwrite setState
, but I might make that even more clear than now, since it's apparently needed
Yes i think it will help using this great class. Explaining what are internal methods and what user can use and create will be better.
Scores are better with your last recommandations for todo bench, from 476ms to 268ms for "creation". Thanks for your remarks.
Hi,
I have created a benchmark suite for Web Components librairies. hyperHTML is implemented, and you can see the raw results here : https://vogloblinsky.github.io/web-components-benchmark/
I will release soon a blog post explaining in detail that, but i want a feedback of many librairies authors before, to be sure i am not wrong in my implementations.
Source code here : https://github.com/vogloblinsky/web-components-benchmark
Can you have a look and give me some feedbacks ?
Thanks