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

Todomvc jquery 03 #140

Closed
wants to merge 8 commits into from

Conversation

flashdesignory
Copy link
Contributor

@flashdesignory flashdesignory commented Apr 13, 2023

alternative approach to #133

This has all the updates from the above, minus the mvc pattern.

Scores:
https://gist.github.com/flashdesignory/16bf15573fc2d33b2a79504a050e99bd

@kara

@rniwa
Copy link
Member

rniwa commented Apr 13, 2023

What's the benefit of adopting this change or the change in #133?

@flashdesignory
Copy link
Contributor Author

flashdesignory commented Apr 13, 2023

@rniwa - the other open pr adds an explicit mvc pattern to the build, which @bgrins didn't want to introduce necessarily, since a typical jQuery build probably wouldn't use it.

This pr adds all the clean-up code that is beneficial to a typical jQuery build, without the additional architectural overhead.

The main difference, which I think is beneficial, is the fact that this implementation doesn't re-render the whole list with every update, which is an optimization that other frameworks / libraries do out of the box.

@bgrins
Copy link
Contributor

bgrins commented Apr 13, 2023

The main difference, which I think is beneficial, is the fact that this implementation doesn't re-render the whole list with every update, which is an optimization that other frameworks / libraries do out of the box.

I agree it's a better implementation, but the primary question is which is more likely to cause performance improvements in the wild when engines optimize it, which may or may not be the more optimal implementation. It's hard to make that assessment since there's a huge range of reasonable ways to implement this, especially with jQuery and Vanilla. My general sense is that unless we have a strong reason to change it we should bias towards what was originally implemented / reviewed upstream - ofc along with version updates and basic quality like formatting, removing unused deps, etc which I (mis)understood as the focus of this alternative PR. But I can see the argument for either way.

@flashdesignory
Copy link
Contributor Author

ah I see - the idea of keeping the original implementation is to allow the browsers to optimize for it.
In that case, yes, I can remove the render optimization within this build and revert to "cleanup only".

It feels slightly unbalanced, compared to the other workloads, which are optimized (even before the version upgrades / cleanup ), but it's more important to have consensus and move forward.

@kara
Copy link

kara commented Apr 13, 2023

I agree it's a better implementation, but the primary question is which is more likely to cause performance improvements in the wild when engines optimize it, which may or may not be the more optimal implementation. I

  • 1 to this. If it's not widely representative of how most jQuery apps were implemented (which may trend toward the original code, since jQuery apps in the wild tend to be older), agree we should switch to cleanup only.

@flashdesignory
Copy link
Contributor Author

ok, the last commit reverts the render update.

Now this pr has the following changes:

  • opt into formatting
  • bug fixing
  • rename of some methods (mostly just for me to be able to follow along better what does what)
  • removal of director
  • package.json and index.html cleanup

@bgrins
Copy link
Contributor

bgrins commented Apr 13, 2023

FYI from a quick spot check this seems to be slower across the board with main. Is that expected (i.e. due to fixing bugs / adding missing features)?

@flashdesignory
Copy link
Contributor Author

FYI from a quick spot check this seems to be slower across the board with main. Is that expected (i.e. due to fixing bugs / adding missing features)?

I reverted another change in the Handlebars template.
The intend was to add a label to the edit input of each item (for a11y reasons), but that resulted in a degrading in performance.

@rniwa rniwa added the non-trivial change A change that affects benchmark results label May 19, 2023
@rniwa rniwa closed this Jul 12, 2023
@flashdesignory flashdesignory deleted the todomvc-jquery-03 branch May 20, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-trivial change A change that affects benchmark results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants