-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Remove component NodeRef #2783
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
Remove component NodeRef #2783
Conversation
this test is done at compile time. there's no node_ref field so it can't be set
Visit the preview URL for this PR (updated for commit a15b5db): https://yew-rs--pr2783-begone-comp-node-ref-vybofh7a.web.app (expires Wed, 27 Jul 2022 16:32:26 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Size Comparison
✅ None of the examples has changed their size significantly. |
#[cfg(feature = "ssr")] | ||
use crate::platform::io::BufWriter; | ||
|
||
/// A virtual component. | ||
pub struct VComp { | ||
pub(crate) type_id: TypeId, | ||
pub(crate) mountable: Box<dyn Mountable>, | ||
pub(crate) node_ref: NodeRef, |
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.
I don't know why is this the case, but the bundle size increase is likely due to the removal of this field.
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.
Removing a field causing increase in bundle size... That doesn't really make sense to me
Do you know why it might be causing that?
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.
I think this has something to do with the size of the struct, you can try to replace it with _marker: usize
and set the value to 0
in the constructor. This should also work.
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.
I ran twiggy and it seems like most of bytes are coming from code that is untouched here:
yew/examples/counter_functional [begone-comp-node-ref]〉twiggy diff dist/counter_functional-931e7c89893382d7_bg.wasm dist-pr/counter_functional-ad2bf05e16727653_bg.wasm -n 10
Delta Bytes │ Item
─────────────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
+510 ┊ yew::dom_bundle::blist::<impl yew::dom_bundle::traits::Reconcilable for yew::virtual_dom::vlist::VList>::reconcile::h897bdbe92f15f88d
-285 ┊ yew::dom_bundle::bcomp::<impl yew::dom_bundle::traits::Reconcilable for yew::virtual_dom::vcomp::VComp>::attach::hbd2d9621b55add0b
+253 ┊ <yew::html::component::lifecycle::CompStateInner<COMP> as yew::html::component::lifecycle::Stateful>::view::h68b91f18b2519e4c
+234 ┊ yew::dom_bundle::bcomp::<impl yew::dom_bundle::traits::Reconcilable for yew::virtual_dom::vcomp::VComp>::attach::hde8285620aedef9a
+150 ┊ yew::dom_bundle::bsuspense::<impl yew::dom_bundle::traits::Reconcilable for yew::virtual_dom::vsuspense::VSuspense>::attach::hb3a4b9fd00446556
+144 ┊ yew::dom_bundle::bnode::<impl yew::dom_bundle::traits::Reconcilable for yew::virtual_dom::vnode::VNode>::reconcile_node::h2888f3017cf1b126
+123 ┊ yew::dom_bundle::blist::BList::apply_unkeyed::hf07a69927390bfbe
+87 ┊ <core::iter::adapters::zip::Zip<A,B> as core::iter::traits::iterator::Iterator>::next::hf228331121bfac52
+60 ┊ yew::dom_bundle::bportal::<impl yew::dom_bundle::traits::Reconcilable for yew::virtual_dom::vportal::VPortal>::attach::h22b2b8a5f486b6e5
+53 ┊ <yew::html::component::lifecycle::RenderRunner as yew::scheduler::Runnable>::run::h84768ff9205d0ffa
+23 ┊ ... and 17 more.
+1352 ┊ Σ [27 Total Rows]
not sure what's happening here
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.
Yes, it's weird. But increase the size of this struct does work.
376K with-marker/router/dist/router-872e26b381599680_bg.wasm
384K without-marker/router/dist/router-514df72ff86f280e_bg.wasm
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.
I am in favour of merging this and removing ref
implementation from v0.20 roadmap.
Other than needs updating the migration guide, LGTM.
a15b5db
Description
Fixes #2505
This is half of #2551/#2567. This PR removes the NodeRef for components. We can add
ComponentRef
in the future.cc: @WorldSEnder @futursolo
Checklist