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

Make key fall back to id for VTags #3480

Closed

Conversation

its-the-shrimp
Copy link
Contributor

Description

Makes an HTML element's id attribute a fall-back value for key, if the latter's not provided. This will allow users to get potential performance benefits without concering themselves with the peculiarities of the framework.

Checklist

  • I have reviewed my own code
  • I have added tests

@github-actions
Copy link

Visit the preview URL for this PR (updated for commit ca9536c):

https://yew-rs-api--pr3480-id-as-key-fallback-ndew6aq5.web.app

(expires Sun, 29 Oct 2023 11:40:08 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 102.908 103.635 +0.727 +0.706%
boids 175.774 176.545 +0.771 +0.438%
communication_child_to_parent 95.388 96.181 +0.793 +0.831%
communication_grandchild_with_grandparent 109.124 110.025 +0.901 +0.826%
communication_grandparent_to_grandchild 105.791 106.744 +0.953 +0.901%
communication_parent_to_child 92.864 93.682 +0.817 +0.880%
contexts 113.500 114.491 +0.991 +0.873%
counter 89.268 90.051 +0.783 +0.877%
counter_functional 90.005 90.842 +0.837 +0.930%
dyn_create_destroy_apps 92.440 93.298 +0.857 +0.928%
file_upload 103.600 104.278 +0.679 +0.655%
function_memory_game 174.675 175.571 +0.896 +0.513%
function_router 352.530 353.891 +1.360 +0.386%
function_todomvc 163.555 164.447 +0.893 +0.546%
futures 227.837 228.809 +0.972 +0.426%
game_of_life 112.282 113.146 +0.864 +0.770%
immutable 188.883 190.748 +1.865 +0.988%
inner_html 86.047 86.536 +0.489 +0.569%
js_callback 113.455 114.295 +0.840 +0.740%
keyed_list 201.029 201.859 +0.830 +0.413%
mount_point 89.269 89.962 +0.693 +0.777%
nested_list 115.902 116.680 +0.777 +0.671%
node_refs 96.365 97.312 +0.946 +0.982%
password_strength 1752.378 1753.164 +0.786 +0.045%
portals 98.466 99.372 +0.906 +0.920%
router 318.460 319.853 +1.393 +0.437%
simple_ssr 144.185 145.184 +0.999 +0.693%
ssr_router 390.318 391.816 +1.498 +0.384%
suspense 119.118 120.070 +0.952 +0.799%
timer 91.816 92.593 +0.776 +0.846%
timer_functional 100.520 101.319 +0.800 +0.796%
todomvc 143.861 144.698 +0.837 +0.582%
two_apps 89.977 90.728 +0.751 +0.835%
web_worker_fib 139.025 139.968 +0.942 +0.678%
web_worker_prime 190.481 191.532 +1.051 +0.552%
webgl 88.572 89.215 +0.643 +0.725%

✅ None of the examples has changed their size significantly.

@github-actions
Copy link

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 356.314 360.381 358.469 1.337
Hello World 10 621.033 626.296 624.410 1.564
Function Router 10 2120.527 2138.329 2127.992 5.976
Concurrent Task 10 1006.384 1008.556 1007.607 0.720
Many Providers 10 1471.750 1495.148 1480.967 9.078

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 319.232 319.618 319.448 0.127
Hello World 10 626.985 631.200 627.726 1.261
Function Router 10 2163.083 2179.491 2172.108 4.644
Concurrent Task 10 1006.832 1008.720 1007.686 0.545
Many Providers 10 1446.767 1473.937 1455.117 8.523

@futursolo
Copy link
Member

I feel this pull request introduces a couple loopholes into the keying logic.

  1. Should <div id="header" /> and <div key="header" /> be considered as the same element?
    (a.k.a: having the same key)
  2. When <div id="header1" /> is being reconciled with <div id="header2" />, should it force a replacement?
  • If yes, changing the id of an element will lose the states of all child components.
  • If no, we cannot introduce id as key.

@futursolo futursolo mentioned this pull request Oct 29, 2023
2 tasks
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.

2 participants