-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Assert there are no circular references #3025
Conversation
the check is costly in release builds and should always fail note that the current PartialEq impl is *not* symmetric! should be fixed as well, with an improved design
Visit the preview URL for this PR (updated for commit 111cb96): https://yew-rs-api--pr3025-perf-node-link-1ppwmv1h.web.app (expires Sat, 17 Dec 2022 13:49:17 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Benchmark - SSRYew Master
Pull Request
|
Size Comparison
✅ None of the examples has changed their size significantly. |
wasm_bindgen does not yet support #[should_panic] see also rustwasm/wasm-bindgen#2286
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Have you tried running the benchmark locally to see if fixes the regression?
Yeah locally it does. Something is really wrong with the github actions benchmark setup:
|
111cb96
I have removed a test for the |
Can the link method be removed entirely? |
Not that I know how. It's important for empty and nested components' to be able to forward their |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Let's merge this change then
* assert there are no circular references the check is costly in release builds and should always fail note that the current PartialEq impl is *not* symmetric! should be fixed as well, with an improved design * remove internal test for cyclic node refs wasm_bindgen does not yet support #[should_panic] see also rustwasm/wasm-bindgen#2286
* assert there are no circular references the check is costly in release builds and should always fail note that the current PartialEq impl is *not* symmetric! should be fixed as well, with an improved design * remove internal test for cyclic node refs wasm_bindgen does not yet support #[should_panic] see also rustwasm/wasm-bindgen#2286
Description
The removed check is costly in release builds and should always fail. Note that the current PartialEq impl of
NodeRef
is not symmetric, it's useful only to check for circular dependencies! This should be fixed as well, with an improved design.Fixes #3024
Checklist