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

Explicit duplicate key check #3785

Merged
merged 5 commits into from
Dec 28, 2024

Conversation

WorldSEnder
Copy link
Member

A discussion on discord shows a hard to read error message when accidentally using duplicate keys in lists.
While this is still not supported with this PR, change this to a nicer error message, identifying the offending index and key in the list.

Example stack trace:

slice index starts at 3 but ends at 2

$__rust_start_panic    @    
$rust_panic    @    
$std::panicking::rust_panic_with_hook::h76c11b8da342ec83    @    
$std::panicking::begin_panic_handler::{{closure}}::h66f67617d4dba49d    @    
$std::sys::backtrace::__rust_end_short_backtrace::h622a24dbddaa3376    @    
$rust_begin_unwind    @    
$core::panicking::panic_fmt::h4fd7a00ecfbf278a    @    
$core::slice::index::slice_index_order_fail::do_panic::runtime::hc8b046ffeba454c8    @    
$core::slice::index::slice_index_order_fail::h99288b888202ded2    @    
$core::slice::index::range::hf7288dd9129769c0    @    
$alloc::vec::Vec<T,A>::drain::h7f8c579b770e112f    @    
$alloc::vec::Vec<T,A>::splice::h14dfdac10dd4d213    @    
$yew::dom_bundle::blist::BList::apply_keyed::h67738f26f9ea56a7    @    
$yew::dom_bundle::blist::<impl yew::dom_bundle::traits::Reconcilable for yew::virtual_dom::vlist::VList>::reconcile::h69c9ea01d4616fbc    @

also a bit more defensive in production code, this should not lead to any slowdown
or changes in code with proper keys
github-actions[bot]
github-actions bot previously approved these changes Dec 28, 2024
github-actions[bot]
github-actions bot previously approved these changes Dec 28, 2024
github-actions[bot]
github-actions bot previously approved these changes Dec 28, 2024
don't set the panic hook if we are already panicking
github-actions[bot]
github-actions bot previously approved these changes Dec 28, 2024
Copy link

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)

✅ None of the examples has changed their size significantly.

Copy link

Benchmark - core

Yew Master

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.635 ns      │ 2.754 ns      │ 2.642 ns      │ 2.647 ns      │ 100     │ 1000000000

Pull Request

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.686 ns      │ 3.678 ns      │ 2.707 ns      │ 2.738 ns      │ 100     │ 1000000000

Copy link

github-actions bot commented Dec 28, 2024

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

https://yew-rs--pr3785-duplicate-key-check-w02i5ixk.web.app

(expires Sat, 04 Jan 2025 16:19:40 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Copy link

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 288.378 291.591 290.495 0.959
Hello World 10 498.577 538.638 505.230 11.881
Function Router 10 1604.646 1648.728 1625.231 14.472
Concurrent Task 10 1004.922 1007.583 1006.013 0.733
Many Providers 10 1156.611 1224.946 1187.634 22.197

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 290.855 295.291 291.591 1.343
Hello World 10 493.999 503.726 496.714 2.871
Function Router 10 1626.060 1656.363 1638.888 10.138
Concurrent Task 10 1005.822 1007.221 1006.428 0.448
Many Providers 10 1157.029 1195.273 1178.013 11.258

@WorldSEnder WorldSEnder merged commit 69e3b5f into yewstack:master Dec 28, 2024
26 checks passed
@WorldSEnder WorldSEnder deleted the duplicate-key-check branch December 28, 2024 16:24
geoffjay pushed a commit to geoffjay/yew that referenced this pull request Jan 25, 2025
* explicit duplicate key check in debug_assertions

also a bit more defensive in production code, this should not lead to any slowdown
or changes in code with proper keys

CI changes:

* force install cli tools over cached versions on version mismatch
* Upload PR information for CI
   see also: actions/upload-artifact#618

misc:

* fix panic in panic
   don't set the panic hook if we are already panicking
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.

1 participant