Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement imperative refs, capturing scopes to child components #2551
Implement imperative refs, capturing scopes to child components #2551
Changes from 24 commits
562e879
d1dd4bc
32dfe0d
ff99444
f8b917e
1f3de5d
560e56e
7ecb58f
fc563ad
7070025
08fb45e
f1908f4
39492bf
3e3c674
00a2e0b
0a4a2ee
d5684bc
0a05ffc
58087d5
c2785c7
fc753ba
9eb2edf
b857074
56fa37d
cbf0dd7
da0c131
4bf5e23
7698970
cbad47b
6c3d655
f229c62
b104b58
895ce20
8e77558
22e6db5
6f9590d
afcbeb2
5dd862a
9deac03
cbabe59
258cc3a
42e1a0c
3eeae19
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Whilst I was originally suggesting to not classify
ref
as a normal prop, however, after @hamza1311 brought up the idea of making theref
as a normal prop, I actually think this is a better approach and leads to a much simpler design.Suppose that if there are 2 refs (e.g.:
ref
andinputRef
) for a single component, how to handle these 2 at the same time?(It's not always the case to simply forward the
inputRef
to the inner input element, as it can be enhanced by adding additional methods to it.)In addition, I don't think it actually provides any benefit of actually separating the handling of these 2 refs especially if you have to set it manually.
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.
There's a semantic difference:
ref
means that this component is responsible for binding the referenced value. Normally, you get a currently referenced value from aHtmlRef
. Hence whycrate
accepts a specialBindableRef
type that allows you to bind (and maybe rebind, I'm not sure at this point, but it could make sense) a value.BindableRef
also could have a method to decay it to a ref the component binds to one of its children, i.e. to forward the ref.But being the
HtmlRef
that this and only this component is in charge of binding, is imo special enough to deserve separate treatment.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.
If you wrap a third party component library that is linked with
VRef
and you have 2 props (ref
andinput_ref
) withref
linked to the container div.How do you expose
input_ref
to the inner input element created byVRef
without creating another component?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.
Added
BindableRef::forward
so the component can capture aHtmlRef
it can itself bind on children returned fromview
.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.
This does not quite solve the question I mentioned.
Suppose there is a component with 2 refs,
input_ref
andref
.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 suppose
VRef
should accept aNodeRef
, too?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 see how this solves the example in previous comment.
VRef
is registered onel
butinput_ref
is supposed to be registered oninput_el
which is queried from theel
byquery_selector
.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.
Ah I didn't quite get what you were asking: The best strategy for this case, imo, would be to expose a struct containing all the referenced elements. Such as
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 forgot to mention that usually a custom element usually will not render its content until its
connectedCallback
is invoked. Which means thatel
andinput_el
are actually made available in different component lifecycle, theinput_el
element will not become available untilrendered()
cycle when asel
must be passed asVRef
for it to become rendered.