Implement imperative refs, capturing scopes to child components #2551
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
WorldSEnder
added
feature-request
|
Visit the preview URL for this PR (updated for commit 3eeae19): https://yew-rs--pr2551-imperative-ref-gvsnyl7h.web.app (expires Sun, 22 May 2022 23:35:53 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Size Comparison
|
WorldSEnder
force-pushed
the
imperative-ref
branch
from
aa85eca
to
4ace53c
Compare
WorldSEnder
force-pushed
the
imperative-ref
branch
from
df8ac83
to
a6526fb
Compare
|
|
You can use twiggy to compare binary generated from master and your branch |
WorldSEnder
force-pushed
the
imperative-ref
branch
from
a6526fb
to
1d9e53f
Compare
WorldSEnder
force-pushed
the
imperative-ref
branch
from
1d9e53f
to
309b93d
Compare
this should shave some size from BaseComponent::create
| /// | ||
| /// We provide a blanket implementation of this trait for every member that implements | ||
| /// [`Component`]. | ||
| pub trait ComponentWithRef: Sized + 'static { |
There was a problem hiding this comment.
Does this exist only to avoid the breaking change of adding type Reference to component? If so, I'd be fine with making that breaking change.
We can add a nightly feature flag that avoids the breaking change in nightly builds using default value for Reference.
There was a problem hiding this comment.
I think we can use Component as ComponentWithRef; with the nightly flag. Can we discuss this in a separate PR?
|
To summarize the current state:
The Html*Elements are behind features flags in Solution 1 would be to enable all relevant flags in the dependency for Yew, which will lead to probably longer build times for anybody not using them (but shouldn't impact code size). Solution 2 would be to somehow detect which flags are turned on and fall back to only allowing
Regarding the bundle size, the increase is mainly due to maintaining both the |
There was a problem hiding this comment.
That is (per component, roughly, can vary a lot) Scope::mount +200B, CompState::new +300B, PropsUpdate +300B. Feel free to investigate yourself, but it's not directly caused by HtmlRef, rather an artifact of separating internal DomPosition from HtmlRef.
#2567 also separates HtmlRef and NodeRef. I don't think separating these 2 types is the culprit of the code size increase.
The code size increases on the path where ref is passed, which should be due to how the ref is passed and the inability to eliminate these code when Reference = NoReference.
(The code size can also be impacted by how many fields present on a struct.)
| /// [`ComponentWithRef::create`]: crate::html::ComponentWithRef::create | ||
| #[derive(Debug)] | ||
| pub struct BindableRef<'b, T> { | ||
| inner: &'b ErasedHtmlRef, |
There was a problem hiding this comment.
Does this mean that the ref must be set in create() due to 'b?
If so, I think there needs to be a way to delay this until rendered().
- function components do not have a way to access
create(), the earliest possible render cycle isrender(). refcan be used to update component states which can interfere hydration if done so beforerenderedwhich means thatuse_imperative_handleshould delay settingrefuntileffects.
There was a problem hiding this comment.
You'd use bindable_ref.foward() to obtain a HtmlRef to bind to an html element during render. I agree that it the binding should perhaps only "take effect" after rendered, but it'd complicate the implementation. As long as we don't have callback refs, just be careful. Refs to Nodes are also only accessible after rendered, so this shouldn't be a surprise to users. In any case, function component can use internal api to set the HtmllRef later
| type Message = Msg; | ||
| type Properties = Props; | ||
| type Reference = Scope<Self>; |
There was a problem hiding this comment.
This does not quite solve the question I mentioned.
Suppose there is a component with 2 refs, input_ref and ref.
#[derive(Properties, PartialEq)]
pub struct MyInputProps {
input_ref: HtmlRef<Element>,
}
// Suppose there is a third-party UI library
// Using custom component as an example,
// but applies to any UI library that cannot be created by directly rendering layout.
// e.g.: React, etc.
let el = document().create_element("my-input");
let input_el = el.query_selector("input");
let html = VNode::VRef(el);
bindable_ref.set(el);
// how to set input_ref to input_el?|
|
||
| /// Use this type as the [`BaseComponent::Reference`] type when a component can not be referenced. | ||
| #[derive(Debug, Clone)] | ||
| pub enum NoReference {} |
There was a problem hiding this comment.
Is there any reason of making a NoReference type than using () here?
Setting ref type to () and expecting it to not be set (in debug_assert_bound) also makes sense to me.
There was a problem hiding this comment.
An empty enum makes more sense to me. This is intended to be publicly visible. That is, you should be able to match:
assert!(no_ref.get().is_none()); // not bound. In fact:
match no_ref.get() {
None => {}, // not bound
Some(what) => match what {}, // exactly no variants to match, can safely ignore this branch
}
The only way not requiring user interaction I can currently see, that works, is using either the And please don't compare it so shallowly with #2567, which contains no code for references on components, hence only has to deal with the two types on |
I am not shallowly comparing this pull request with #2567. I realise that these might not be possible with this approach and is happy to overlook them. However, If the code size is not solvable, then it's a limitation of this current design that should be factored in when evaluating this pull request. This pull request is trying to enforce situations that does not always hold and that has resulted in significant complexity in the design to provide escape hatches.
|
|
Closing this for now, since #2783 has been merged. A future PR can introduce the component ref. |
Description
Instead of getting a ref "the first element", now get a link to the component the ref is attached to.
This is a breaking change. Components wanting to preserve the old behavior should add a property and forward that.
There's probably a bit of documentation to adjust, I haven't check all places yet.
Fixes #2505
Fixes #1257
Fixes #905
Fixes #2197
Related #905
Checklist
cargo make pr-flow