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 uhtml dependencies and logic #908

Merged
merged 1 commit into from
Jul 3, 2021
Merged

Update uhtml dependencies and logic #908

merged 1 commit into from
Jul 3, 2021

Conversation

WebReflection
Copy link
Contributor

@WebReflection WebReflection commented Jun 30, 2021

This MR applies the following changes:

  • previous techniques are preserved
  • the 801 "issue" has been removed
  • new helpers landed as dependency update
  • some logic has been moved (as implementation detail) to the helper
  • events are attached directly to the element, like others without 801 do
  • no trickery beyond the events ... these are not a top level delegate, even if that would give more perf boost

Thanks for considering this MR 👋

@krausest krausest merged commit f68e0ab into krausest:master Jul 3, 2021
@krausest
Copy link
Owner

krausest commented Jul 3, 2021

Nice! What from the above explains the significant improvement for select row?
Screenshot from 2021-07-03 21-32-04

@WebReflection
Copy link
Contributor Author

@krausest apologies for the late reply. As the whole benchmark is based on a single id (one selected row per time, never more) the select row logic has been moved into "implementation detail", like it is for a few others libraries. The shared/common helper logic that drives the benchmark logic won't "move the world" on row selection changes.

I don't know if this needs to be flagged somehow, but 801 is gone (although I forgot to update the non-keyed bench too).

Happy to provide more details, if needed, and thanks for the merge. Non keyed might come soon too.

@Freak613
Copy link
Contributor

@krausest @WebReflection it looks like this implementation should be moved into #772 category, based on code from its dependency:
https://github.com/WebReflection/js-framework-benchmark-utils/blob/master/cjs/index.js#L108
We have a lot of other projects in this group. It is clear that current implementation is(or may be) idiomatic for this library, but current categories are the way it is. There is long discussion on the topic, so further suggestions and thoughts can be put there to keep others involved, while here we can discuss disagreements whether or not this implementation should be moved into this group.

Also, I feel that moving whole benchmark implementation into current repository will ease further reviews, without need to manually scan through each dependency for the actual code.

@ryansolid
Copy link
Contributor

ryansolid commented Jul 28, 2021

I didn't pick up on that. That repo of utils especially select row is clearly #772. It's hidden from the in repo implementation but it is also not library internals and clearly is tailored for the benchmark. It stashes the DOM element and toggles the classList directly with "danger".

@WebReflection
Copy link
Contributor Author

I feel that moving whole benchmark implementation into current repository will ease further reviews

Various libraries of mine use that very same dependency, but in more than one occasion I've stated that we should have shared benchmark logic across libraries so that all have the same underlying performance, and we'll measure, at that point, only libraries overhead.

I am OK with the #772 flag though, as I've already mentioned before.

I don't know if this needs to be flagged somehow, but 801 is gone

This benchmark does not allow multiple rows selection, so that looping through all of them makes no sense. Previous versions are still within the uhtml folder.

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.

4 participants