Skip to content
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

Update lit-element to Lit #877

Merged
merged 1 commit into from
Apr 25, 2021
Merged

Update lit-element to Lit #877

merged 1 commit into from
Apr 25, 2021

Conversation

ryansolid
Copy link
Contributor

New Lit version. Early testing seems to indicate nice improvements in performance and size. Memory reduction is especially impressive.

@ryansolid ryansolid changed the title update lit-element to Lit Update lit-element to Lit Apr 22, 2021
@Freak613
Copy link
Contributor

Speaking of memory reduction, are we going to mark it with #801 flag ?
See: https://github.com/krausest/js-framework-benchmark/blob/master/frameworks/keyed/lit-html/src/index.js#L29

@ryansolid
Copy link
Contributor Author

It's a fair question. We haven't been marking 801s as far as I know. Personally I felt this one more controversial in that whether the library does it or the end user this is still going to happen.

Web Component centric libraries are unlikely to support this out of the box given challenges with the Shadow Dom and being built to co-exist. I actually support Shadow DOM automatic event delegation in my libraries, and it has had some friction but I side on the perspective I prioritize the ability to use it like a framework but it has definitely bit widget use case. I ended up downscaling it a bit and only delegating certain events to appease them, some would rather it not be implicit at all.

A library like Lit will never do implicit event delegation for this reason but sees no problem encouraging others to do it. A number of libraries even have the technique in their official docs, so it feels weird to mark them for a technique everyone is using anyway.

But I've shared this opinion before. We could still mark these anyway as an indicator, almost like feature detection.

@krausest krausest added the merging started merging started (no more updates please) label Apr 24, 2021
@Freak613
Copy link
Contributor

Just like #800, any library, even with automatic event delegation, would benefit from it. This will reduce memory consumption and housekeeping cost.
Currently we have only apprun marked, but PRs can be good reason to review for what is left in the list.

Speaking of Web Component focus, if rows would be implemented as WC with closed Shadow DOM, there will be no way to dive into row internals from outside, to do what handler does in this benchmark. So it seems to diverge from library main purpose and I can't see them encourage people to do that, simply because of amount of problems with encapsulation it's going to introduce.

@ryansolid
Copy link
Contributor Author

You are right manual event delegation is faster than automatic. The difference with 800 to me was when we say no it doesn't happen. Everything is more or less equal. When we stop 801 the libraries with automatic have a sizeable advantage. To be fair I don't have a personal stake in this since my libs do automatic. Maybe its time to mark 801s.

This one is definitely an 801.

@Freak613
Copy link
Contributor

It's same kind of advantage as template cloning vs building each DOM element separately. Cloning will have advantage but can not be easily ported into non-DOM environment. Automatic event delegation will have advantage, but may suffer of problems with Shadow DOM, active/passive event listeners or integration with third-party/legacy code. It's a matter of balancing use cases and limitations. Maybe one day browser vendors introduce another feature incompatible with delegation or just make native listeners as fast and everyone will be happy.

@krausest krausest merged commit 4d616f2 into krausest:master Apr 25, 2021
@krausest
Copy link
Owner

Thanks. Here's a screenshot from the results:

I added #801 to lit.

@krausest krausest removed the merging started merging started (no more updates please) label Apr 25, 2021
@ryansolid ryansolid deleted the lit branch April 26, 2021 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants