Add Event Delegation to Svelte Implementation #561
Conversation
| swapRows = () => { | ||
| if (data.length > 998) { | ||
| data = [data[0], data[998], ...data.slice(2, 998), data[1], data[999]]; | ||
| } | ||
| }, | ||
| click = ({ target }) => { | ||
| const id = Number.parseInt(target.closest("tr").firstChild.firstChild.nodeValue); |
There was a problem hiding this comment.
so, we're still doing this, huh?
There was a problem hiding this comment.
Fair is fair? I added implicit Event Delegation to my libraries to align with what the VDOM implementations. And I like the story it tells for code aesthetics ("Write less code"), but it is hardly a fair comparison when implementations aren't allowed to use the exact same mechanisms.
I realize the same could be argued about any trick VanillaJS does, but it just means the floor is pretty low. It's the same old discussion again. Which for me comes down to how does the implementation want to present itself. As long as the minimum requirements are met (Ie.. completely data driven, ie renderable from non-DOM state at any given point) it is up to them how nasty and complicated they want to make their implementation.
So yes I was hesitant to do this initially. Especially without @Rich-Harris approval since I get the impression Svelte is a library that cares more about aesthetics than performance. But absent of that I'd say it's probably fair to align this with other high performing non-VDOM implementations like Surplus, LitHTML, LighterHTML, and so on.
As always ready to open this debate again. But I think it is very difficult for any implementation/implementor that cares about performance and doesn't include implicit event delegation in their libraries for various reasons, to even understand why this is an issue in itself. I implemented implicit event delegation and I still don't.
|
Here's the difference we're be talking about: |
|
Yeah that's significant. More than I thought (I'm sort of writing implementations blind right now). It puts it right in the mix with the others. Given that gain I'm gathering for the cost in aesthetics, all being fair, this represents Svelte the best. I'm sold. Does anyone want to veto this? |
|
i'm not brave enough to cast a veto vote. i think the vdom vs non-vdom distinction is irrelevant to whether this strategy should be acceptable, since most vdom impls can do the same thing. we just have different places where we draw the line for what constitutes a fair representation of lib perf comparison. i think by now everyone's aware that i'm in the camp of implicit delegation (that's baked into the lib) rather than manual dom crawling and readbacks in the impls. i had this in domvm's impl long ago, and removed it by adding implicit delegation and taking the small perf hit in favor of ergonomics. |
|
I'm impressed with the difference! |
|
To be honest: I'm not sure how I should handle this request. If it was just for svelte and would serve as a reference on how much this benchmarks benefits from implicit delegation I could certainly live with it, but I'd like to keep both versions: The old version and an "-optimized" version with explicit event delegation. I thought a while about allowing a "-optimized" version for all frameworks. Those versions would be allowed to use explicit event delegation (or any other gradual transistion to becoming vanillajs - are there any rules left for them?). The next logical step would be to add a filter in the UI to remove -optimized results... If I end up with most frameworks having two versions, maintenance would become much harder, so maybe a rule will be needed that only some (important) frameworks should be allowed to have an optimised version (angular has that privilege already btw.) I think I'll come back to it when I've decided how to run the benchmarks on my new (well, not really new anymore) notebook. |
|
@krausest I understand the difficulty here. I think you are correct not to merge this. Unfortunately I doubt there will ever be agreement on event delegation. I believe that there is a clear line that can be drawn for this benchmark as highlighted in the first post of #430. We had very little difficulty convincing offenders to reach that line, but event delegation could not be agreed upon. But it also became clear from the last round there is a sizeable group that feels that line is way low to be considered acceptable. I also don't think this is an optimized/non-optimized scenario as it affects every Framework in this benchmark. It isn't some specific tweak for Svelte. Having both isn't worth the complication brings. The problem here is anecdotally you are looking at a spread something like:
You are penalized way more for not using Event Delegation than you are for small differences on how you do it. That's why when one talks about the cost of their implicit event delegation (and I've done this myself, guilty as charged) it really is of no consequence compared to being blocked from even joining the party. I created this version since I knew some people were discouraged by the Svelte 3 initial results and didn't understand why Svelte was so much slower. The answer is, this wasn't apples to apples. As to whether it should be is much more complicated. I've done what I can to draw people's attention to it. I'm fine closing this and letting @Rich-Harris or the like submit something if it's important to them. If this goes another week or so I think will just do that. |
|
@ryansolid for libraries like |
|
@localvoid |
|
@ryansolid like any other data binding, it won't be able to just clone a node with its subtree, it will need to find a node and assign an attribute, property or add an event listener and the overall cost of data bindings is way much higher than the difference between implicit event delegation and |
|
@localvoid Isn't that like anything? I mean I'm looking at domc with it's parameterized event delegation. It still needs to walk the tree to assign the property it will be using "__click" in this case. Solid is essentially the same. I walk then statically assign the property once on creation. I'm gathering the argument is that lit-html does a lot more. So I shouldn't assume the event handling is as simple as imagined. And that is why event delegation makes a larger difference on some libraries compared to others. So the concern is that allowing explicit event delegation masks the fact that these libraries may have significant overhead attaching event listeners. |
|
Closing due to lack of interest from Svelte Community and recognizing short of that this won't get pushed through. |
This commit adds event delegation to the Svelte 3 implementations as suggested by #557.
I kept this on Svelte 3.2.0 since there are some performance issues with the current version as noted in: sveltejs/svelte#2768 and I don't want to take any risks in compromising performance.