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

jQuery test only ever marks the first todo item as completed #365

Closed
rniwa opened this issue Jan 27, 2024 · 6 comments · Fixed by #367
Closed

jQuery test only ever marks the first todo item as completed #365

rniwa opened this issue Jan 27, 2024 · 6 comments · Fixed by #367
Labels
bug Something isn't working non-trivial change A change that affects benchmark results v3-blocker

Comments

@rniwa
Copy link
Member

rniwa commented Jan 27, 2024

Step through jQuery suite in https://speedometer-preview.netlify.app/interactiverunner?suite=TodoMVC-jQuery

We observe that the only first item is marked as completed in "CompletingAllItems" test.

@rniwa rniwa added v3-blocker bug Something isn't working non-trivial change A change that affects benchmark results labels Jan 27, 2024
@rniwa
Copy link
Member Author

rniwa commented Jan 27, 2024

The issue here seems to be that the entire todo list is re-generated each time any checkbox is toggled. So we need to execute querySelectorAll in each iteration, or update jQuery implementation to not nuke the whole list each time any todo item's completeness is toggled.

rniwa added a commit that referenced this issue Jan 27, 2024
…odo item as completed

The bug was caused by jQuery replacing the whole todo list whenever each todo item is toggled.

Fixed it by running querySelectorAll in each iteration.
@flashdesignory
Copy link
Contributor

we had several prs closed for jQuery, both would address the whole list re-render issue:

#133

#140

comment from me from the last example:
"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."

Happy to take a look at both and open a pr for an updated version!

@julienw
Copy link
Contributor

julienw commented Jan 29, 2024

Another possibility would be to use getElementsByClassName which is a live collection. This might change the performance characteristics of the workload though.

@rniwa
Copy link
Member Author

rniwa commented Jan 30, 2024

Another possibility would be to use getElementsByClassName which is a live collection. This might change the performance characteristics of the workload though.

Yeah, that's another alternative. I'd be happy with either approach.

@flashdesignory
Copy link
Contributor

I can take another look at my previous ones and open a new pr for opinions?
Should be able to do that later today if that works.

@bgrins
Copy link
Contributor

bgrins commented Jan 30, 2024

At this point, my preference is to take a change like #366 as opposed to a broader refactor to optimize re-renders. I'd like to make an as-narrow-as-possible-correctness-fix for 3.0, and to consider whether we should make a refactor for a future version.

julienw added a commit that referenced this issue Feb 8, 2024
…eration (#367)

This PR fixes the issue #365: jQuery test only ever marks the first todo item as completed

The bug was caused by jQuery replacing the whole todo list whenever each todo item is toggled.

Fixed it by running querySelector with :nth-child in each iteration.

Co-authored-by: Ryosuke Niwa <[email protected]>
jrmuizel pushed a commit to mozilla/Speedometer that referenced this issue Jul 23, 2024
…eration (WebKit#367)

This PR fixes the issue WebKit#365: jQuery test only ever marks the first todo item as completed

The bug was caused by jQuery replacing the whole todo list whenever each todo item is toggled.

Fixed it by running querySelector with :nth-child in each iteration.

Co-authored-by: Ryosuke Niwa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-trivial change A change that affects benchmark results v3-blocker
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants