-
Notifications
You must be signed in to change notification settings - Fork 854
Note: Implementation uses explicit event delegation [sticky] #801
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
Comments
Any progress on this issue? I’m realizing that assigning DOM Also, does anyone have any microbenchmarks for whether |
We don't seem to be marking libraries for this currently. So I don't believe there is any repercussion to doing so? Atleast today. #772 is more about manipulating the selection class directly. I've argued against this being penalized since it is the defacto way to solve events in a large table. Either the framework does it, or you do it. Some libraries actually have this in the documentation as being the way to do it. Once you get to a certain point on the table everyone is using event delegation. It isn't like it is fundamentally changing the nature of the test. I think this is sort of gray area in that it makes the implementation look less clean so it's discouraged. It breaks the abstraction wall. But it's hard to penalize someone for something that almost every other library is doing. I do see the argument that if we are critical on libraries using imperative escape hatches to directly manipulate DOM elements in ways the library already has means to but are being avoided purely for performance reasons, that being fine reading from them is a very subtle distinction to make when the library already has a way to attach events. To be fair explicit event delegation still generally uses the libraries event attaching mechanism, it just involves manual traversal of DOM trees. But I think we need to decide if this is something worth penalizing on. And if not, leave to the implementor. I once made an event delegated version of Svelte but I wasn't comfortable merging it without Svelte community or Rich's blessing. I never got it so I retracted my PR. |
It’s odd because it’s one of those optimizations that anyone working with large amounts of DOM nodes will go to first, but I understand the arguments against explicit event delegation. Specifically, you break encapsulation between your table and row components if the row cannot hide its internal DOM structure because the table is reaching into the row during click events. Then again, I don’t think any “row component” should be written in isolation from a
Is it just me or does Svelte have an even stronger case for event delegation because it doesn’t define a separate component for rows? |
@brainkim |
I wouldn‘t call it „penalize“. It‘s not an error like #694, it‘s just a note that the implementation might take advantage from a user code event delegation. |
In the end I decided against doing event delegation because it wouldn’t be in good sport. Also finding the row id and making sure the target contains a specific tag seems non-trivial. |
This issue requires revision, since aside from opinionated labeling, it currently lacks any value.
The reasoning behind this is below. Historically this label, and actually any label here, was viewed as a problem, even now when this was changed to "Note" it is viewed as a bad spot. |
I disagree. The thing is every framework can escape out of its mechanics to do something more effectively here by just using the underlying DOM and JS. That makes the benchmark uninteresting. That was the problem the benchmark was having. Solid used to not do event delegation, so we'd do this thing where we captured the element and looked for the closest TR etc.. it was very bulky. Then you'd have React which just applied an event handler. Now React could have done explicit event delegation too and it be even more performant but who would do that when the built in was pretty good?
Now I get the argument that now automatic event delegation is seen a very different light than 4 years ago. It very much was seen as important because of historical fact that it alleviated issues like differences between browsers off the end developer. These days it has become so ingrained that it is pretty fundamental modern frameworks to handle things like Portals and optimal hydration. So it can be argued that event delegation within the confines of the framework is if so chosen a very good thing. It's an interopt tradeoff. I think there could be a flag for any event delegation but the one in this issue is specifically to point at doing manual DOM operations in user space for the sake of performing better in the benchmark. That is important to be aware of. Although personally I never let this one discount implementations in the benchmark. We can have opinions whether or not frameworks should be using event delegation but whether this benchmark is concerned with that is debatable. |
This benchmarks gives the best insight when implementations look like in the framework's docs since all other implementations converge against vanillajs. |
I think this argument now is obsolete, it may have been so in the early days of existence of this benchmark, nowadays even the most miserable frameworks have enough abstraction for masking manual DOM operations, yet having perfectly working delegation outside of the framework, which cannot be classified as explicit by the above definition, either implicit. Thus now explicit is specifically about the delegation as a feature in user space and not its implementation details. Maybe there are few cases when manual DOM operations are used to delegate events but those would be more appropriate for 772. |
This comment made me to dig into the docs of certain frameworks, though that was mostly a waste of time, I have yet to see such framework's docs specifying the idiomatic style of event handling/delegation. To my knowledge it is always at discretion of user which style/paradigm to adopt. It seems that here the concern is more about style than technical details, because it's incomprehensible how delegation, which is a programming pattern, can converge into vanillajs which basically means JavaScript API. This is like saying that a song coming from an outdoor speaker is explicit sound compared to indoor speaker, and it converge into singer. |
Please take a look at the new implementation of strve-rv: https://github.com/krausest/js-framework-benchmark/blob/master/frameworks/keyed/strve-rv/src/main.jsx#L122-L131 |
Thank you for pointing out the point of explicit event delegation. I did not display the mechanism of automatic event delegation in the framework. I would like to enable users to manually optimize it. I specifically wrote this code for benchmark testing. If we consider the accuracy and rigor of benchmark testing, then my approach is indeed not very good for other frameworks that have done automatic event delegation. We should not excessively manually optimize for the sake of optimization, as it would lose the meaning of testing. |
This "rule" penalizes basically not only implementations but the underlying frameworks for not having built-in delegation, forcing the encapsulation style of events definition, which by the way is an anti-pattern from the performance point of view, and this whole thing is reasoned as cutting corners. I just wonder, what if writing like that in a framework which have implicit/hidden/automatic/whatever non-explicit delegation, performance wise it would not matter and there would be only a delegation style concern, would that also be regarded as cutting corners? If not then maybe a new label is needed for that, like "implementation uses non-idiomatic style of event delegation"? |
so we are witnessing exactly this:
as a result: #1770 A perfectly valid code was adapted just to comply with the style "rule". Now that will be a suboptimal/underperforming code no matter what approach was used under the hood. window.addEventListener('click', e => e.target.onClick?.(e)); And that is what now this label is asking for, do your weird thing in your framework and we are good, trying to be delicate and explicit - here is your #801, you deserve it. |
This reminds me of the difference between manual and automatic transmission in cars. |
That comparison is not even a close, the difference there is huge technically, not that much stylistically. Here the talk is just about the look of a naked quacking duck (read delegated event). |
Benchmarking is not only about performance scoring, but also about promoting one's own framework. |
I don't think such discussions should get into this thread but I will reply as an exception. |
This note (and it should be regarded as a note, not an issue) marks implementations that use explicit, i.e. manual, event delegation.
The note is somewhat controversial since there are multiple views on it:
Advice: Use whatever fits the idiomatic style of your framework, but please don't over optimize. This note adds a litte pressure to prevent over-optimizations.
The text was updated successfully, but these errors were encountered: