-
Notifications
You must be signed in to change notification settings - Fork 842
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
New rule: Prevent direct DOM access #522
Comments
Proposal 1: |
Proposal 2: |
Proposal 3: |
@krausest mind expanding on the explicit event delegation? Both lit/lighterhtml use the row to intercept once internal events, while hyperHTML, as example, places listeners right on the node that should trigger the action. |
P.S. I like all proposals but I'm not sure I get the last one |
@WebReflection Maybe this way: Proposal 4: |
yeah, there are a bunch of libs that delegate and dom-climb (in the impls): https://github.com/krausest/js-framework-benchmark/search?q=parentNode&unscoped_q=parentNode |
@krausest I like proposal 4 the most 👍 |
this part is gonna be the most controversial. i feel like outright disallowing it is a bit heavy-handed, but policing the details gets messy in a hurry. with the right abstraction, it could probably be acceptable as long as it's generalizable for other applications and is uniform for create and update. for example, if the framework had a way to notify of the granular necessary |
This conversation has gotten so far into the weeds. The point of DOM libraries is not to abstract all access to the DOM. It's to help people build useful apps. There are lots of ways to do that. All have their respective pros and cons. Some people -- yeah, I'm calling out @localvoid here -- have gotten so far into the VDOM approach that they have tunnel vision. They demand that all other libraries adopt the same cons VDOM faces or they're "cheating." I ask a library to do what it does well, and for the rest, to get out of my way -- which is to say, to interop with non-library code in predictable ways with minimal ceremony. The fact that Surplus (and lit-html) plays well with the DOM is a feature, not a bug. (Aside: Web Components are built on DOM nodes as an API. If you put this rule in place, you're saying Web Components are wrong. Frankly, I agree that Web Components are a bad design, but not for this reason.) None of us are doing original research here. All these approaches were tried in the last millennium, most in the 70s. If there was a clear winning strategy it would have won 30 years ago. Surplus is based on research done by INRIA in the 80s and 90s into declarative, predictable effects. It is idiomatic to work with the DOM API in Surplus, because Surplus intentionally makes it possible, and S computations let you do it in predictable and declarative ways. /rant I would propose a different rule:
(Put-up-or-shut-up: https://github.com/adamhaile/surplus-realworld ) I think this rule would filter out a lot of the libraries that were built just for this benchmark. I support people exploring ways to achieve awesome performance, but they shouldn't be in the benchmark until they've proven they can build useful apps. |
@adamhaile a repo called real-world doesn't mean every library is used in the real world. hyperHTML ships with ABP to million users and it's been used by W3C Respec for long time and I've never had the time, or the interest, to PR real world cause I've done all the others already plus the lib literally ships to the real world so your proposal is also kinda irrelevant. |
@leeoniya I think if the way of the library is to manipulate directly the Dom then let it be? This new rule purpose is more about avoid cheating for the sake of scoring. |
@WebReflection Yep, the things you cite are exactly the kind of proof I was referring to. They show that hyperHTML is a mature and useful lib. I cited realworld as an example of a minimum requirement -- hyperHTML is obviously way over that. My comments were directed at some of the libraries that don't seem to have any use beyond submitting entries to this benchmark. Those libs should absolutely be implementing this benchmark for their testing, but I would argue that they shouldn't submit until they've developed the lib to the point that it can build useful apps. |
@adamhaile , correct me if I'm wrong, It's not in the demo, because optimization requires additional effort to find tight places and apply fixes. You can say that in React, when we put sCU we also need an effort for that. But in React, sCU shows feature of the library itself, how it can address dataflow optimization. And as a bonus, we can port it to other platforms, and still utilize its benefits. S.js is the same, cause of data-centric approach. Most of the libraries, if not all, have exit gates to the DOM. And if not Ref-s, access to However, I agree about matureness requirement, other way the benchmark will soon be full of one-day libraries. And my libraries in this bench can be considered as that (if don't look at the numbers of npm downloads), although they're not using techniques described here, working in the designed way. That's what stops me from pushing any new raw libraries here, until I test it enough to decide that I reached sufficient functionality. I sure you, if we're thinking that manual delegation and lifting is best what we can, that's not true. It's limiting us. There should be ways to generalize such approaches, and instead of arguing about what level of workarounds is acceptable in this or other benchmark, it's better to face that there is no perfect library, there always be weak points and edge cases, and start thinking about how to fix that. Library developers should carry over as much complexity as possible, there was no VDom 10 years ago, but now it's here and it's helpful no matter how bad it is at some points. |
I'm all for proposal 2, but that should be no surprise. After all the discussion in #430 only the stipulations I made in the first post were never contended. I feel any of those rules would be fine to set. The rest of the discussion goes off about exposing DOM, categorization, and degree of severity of different bad things one can do. But it was never my intention. I wanted to focus on an agreeable baseline rather than argue about how we are all different. To me matureness is pretty hard to enforce. We can stipulate again some very very basic constraints but they'd be too loose for those that care. My personal opinion is a library is only misrepresenting itself when it doesn't have means of control flow abstracted into the library or it hasn't addressed it's disposal logic. If your library can handle mounting and unmounting conditionally and rearranging lists, without creating memory leaks, it's enough. We definitely have had libraries that don't have the primitives to make real apps, and there is definitely some libraries that are memory leaking right now. Most obvious indicator is when test 24 and 25 have relatively the same high memory usage. None of the tests, test conditionals. I'm gathering there was a show/hide at one point that did. Largely redundant outside of it ensures the library to do conditionals. EDIT: Thinking about it TodoMVC. If your library can do the official TodoMVC. Realworld app I've looked at and I've wanted to do but I imagine it would take me days if not weeks given a small windows of time to work on it. TodoMVC isn't as full featured but it contains all the core pieces for the library to be functional. But while we are talking about how we are all different. I know why the Top Down render libraries see just updating it outside of the render cycle so taboo, it feels hacky to just go in there after the fact and mess with the DOM even if it was masked from the implementor. Even if you compile the view into separate creation and update paths it is something fixed you don't touch that runs systematically from top to bottom. In fine grained, all rendering works like the hack. There is no render cycle. Everything is after the fact (after the initial render). Any part of the DOM updates independently. You find yourself writing directives to encapsulate DOM manipulations as that is the same foundation the whole rendering is built upon. Whether I'm binding to update an attribute or defining some complicated reusable behavior it is more of the same. It's an event system more than a rendering engine. It's the same reason I can take Knockout and use it completely differently with ko-jsx. With a few primitives you can change how any part of it works.
That is exactly what directives are. It's been idiomatic to do that in fine grain libraries forever. I'm glad @localvoid has illustrated how in general every library can do this though because it brings to light the fact the ability to do so isn't necessarily a feature. However it doesn't mean it isn't more idiomatic for some libraries to approach things this way. That being said I'd love a future where end user directives were not a thing as it definitely is more cognitive overhead for developers. On the other hand event delegation is one that I've been struggling with a lot since generally explicit event delegation is an optimization technique you only use when desired. It doesn't usually feel free. When I'm using webcomponents I use it a lot more. When I'm not less so, since I'm generally not exposing custom events. Being able to define the handler methods on the children rather than the parent make a lot of sense. but is really difficult for single pass libraries that have to consider the ShadowDOM. If you haven't attached the node to the parent how do you know which shadowroot to append the delegate to. The only reasonable option is explicitly add the event listeners for the node handling delegation but that requires more verbose bindings across multiple elements to pull off. I always link the page in Knockout docs where they tell you how explicit event delegation is what you want to do in these scenarios in their fine grained library. The truth is you won't usually use it unless it's called for to optimize, at which point you will do so automatically and assuredly. You think no worse for doing it. Is a benchmark a place where you'd optimize? I think so. Is it idiomatic? Sort of, it is the recommended solution for the problem. It's what anyone using the library would do in that scenario. Any library could do it, but would they? So what are we benchmarking? Is this a comparison of different render techniques or a showcase of how someone would use the library if presented with the problem? I realize we lose if everything just digresses to vanillajs, but many libraries that don't have implicit event delegation will almost certainly use explicit when the right scenario comes along. It's part of the toolbox. |
I think it also matters for whom we are benchmarking. Implementers leverage this benchmark to have a feel for how their optimizations affect several measurements in simple scenarios one can reason about. In this case, one can just run it locally without worrying about how it performs against the other frameworks. We've done this a fair bit in Reflex-DOM to check for significant performance regressions in some large changes. As for consumers of the lib, I imagine they're mostly concerned with
I can't help but feel we ought to be measuring both extremes: allow frameworks to have a version where all the abstractions that make sense are used, and an every-trick-in-the-book version. That way folks can judge whether the 'elegant' one is too slow or the 'fast' one is too difficult to work with for their purposes. Right now we force each framework to pick one point in this spectrum and then have a hard time of getting everyone to agree on which point should be chosen, weakening the value of the comparisons. |
Probably, one more option is to mark in the result table whether or not library used access to DOM from the userspace and let the people do whatever they want. People who referring to benchmark results will immediately get information about how good library handle bottlenecks and do they need to write manual updates. |
possible candidates for indicators:
until the details are hashed out, can we all agree on Proposal 2 (the initial rendering must be able to render the selection state.) and maybe perhaps that are there any thoughts on whether caching |
Having marks, need for any new rule becomes not so critical. From real world standpoint it's unlikely you have selected element for not rendered list, and the benchmark doesn't test this case, therefore making this requirement restrictive just to make a restriction. The short recipe for unrestricted environment will be:
And the whole place will burn. |
I would put just one mark, is the implementation fully Data-Driven or not. UPDATE: okay, not suitable to filter out DOM access. Then any term/description about restrictions from this and similar tickets. |
What I'm getting at is that the state at any point can be rendered from the data not stored in the DOM is the only definition that universally sticks for what being "data-driven" means. @Freak613 when you say fully data-driven what do you mean? I don't believe precludes the potential of DOM manipulations and caching as libraries do that under the hood anyway, it's a matter of chosen exposure/abstraction. Being data-driven doesn't exclude imperative manipulation, just says that the state belongs in the input not in the output of: const view = fn(state); fn could be a lot of things and still be data-driven. |
@Freak613 just wanted to highlight this point to agree 1000%. That's the spirit that I wish prevailed here. The Surplus implementation here hasn't changed for several months. That's not because I think it's perfect. I often pursued a "ratcheting" strategy: make it fast-but-manual to put a line in the sand, then make it fast-but-clean while achieving the same performance. Frankly, the level of trolling got high enough here that I took my time elsewhere. All approaches have their pros and cons, but some here think that their approach's cons are more important than others, and if other libraries don't adopt the cons they have to face, then they're cheating. Meanwhile, we're not even talking about implementations that:
But DOM access -- even though your library is based on an entire area of research (SRP) oriented toward making predictable, declarative side effects -- that's beyond the pale. |
The voting wasn't exactly helpful to select a new rule 😬 . |
Please vote "thumbs up" for the formulation of the new rule that should be added to the benchmark or propose a better formulation:
The text was updated successfully, but these errors were encountered: