search

WebKit / Speedometer

An open source repository for the Speedometer benchmark
Other
615 stars 73 forks source link

NewsSite-Next workload is generating keys for <li> element on the fly #422

Open lingyuncai opened 2 months ago

lingyuncai commented 2 months ago

Since the data doesn't contain an id field for each item, the workload is now generating keys for each <li> element dynamically while rendering, using uuidv4(). https://github.com/WebKit/Speedometer/blob/3150f4117389931ba36871c4a0ab4e289d273965/resources/newssite/news-next/src/components/article/article-content.jsx#L23-L32

This will cause keys to never match up between renders, leading to these elements being recreated every time, which defeats the purpose of keys and slows the performance [1].

Besides, this implementation seems inconsistent with that of NewsSite-Nuxt, which uses item.id (which is undefined) as the key binding directly. https://github.com/WebKit/Speedometer/blob/3150f4117389931ba36871c4a0ab4e289d273965/resources/newssite/news-nuxt/components/atoms/ArticleContent.vue#L29-L34

[1] https://react.dev/learn/rendering-lists#rules-of-keys

lingyuncai commented 2 months ago

PR to address this issue: #423

flashdesignory commented 2 months ago

@lingyuncai - thanks for the issue and your solution. I opened a separate pr, where I added the unique ids to the actual data files. In that way we don't even need to generate them at any point in the app.

https://github.com/WebKit/Speedometer/pull/426

lingyuncai commented 2 months ago

Hi, @flashdesignory

Thanks for your new PRs, they indeed solve this issue in a more clean way and it looks great!

Once these PRs are merged, do you know which version of Speedometer will include these changes?