-
Notifications
You must be signed in to change notification settings - Fork 344
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
fix(reactivity): unwrap nested refs on the template #361
Conversation
Co-authored-by: Anthony Fu <[email protected]>
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.
Thanks!
LGTM. If you are ok with it, I can do the resolving and merge it now. |
Sure :) |
fdf8fe5
to
378104e
Compare
return value | ||
} | ||
|
||
if (isReactive(value)) { |
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 don't mind answering, I would really be grateful if you take the time
seems to me, that we have a base-case if value
is reactive
, and we don't recursively unwrap
but on the test-cases it doesn't seem to be the case:
https://github.com/vuejs/composition-api/pull/361/files#diff-ffb3fccc5cc98e58adb1477f88e460b83b048c4b693b3fdb479a559f0c66cb88R484-R490
asking because i'm running into a reproduction and for a minute i was tricked by a premature understanding of this check
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.
reactive
has some caveats on Ref Unwrapping
but if a value is reactive, you can safely assume that it's already unwrapping.
This commit is pretty old, some things changed since them
<p id="nested_aaaa_bbb_cc">{{ nested.aaaa.bbb.c }}</p> | ||
<p id="nested_aaaa_bbb_cc">{{ nested.aaaa.bbb.cc }}</p> |
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.
same id, different ref, and confused with :528 which shouldn't unwrap - the tests for those 3 cases are wrong (overwriting one another)
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.
Nice catch. Would you mind open a PR? Thanks.
Breaking change
This unwraps nested refs on the template
NOTE: this needs to be intensively tested