Skip to content
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

Merged
merged 7 commits into from
Jun 10, 2020

Conversation

pikax
Copy link
Member

@pikax pikax commented Jun 9, 2020

Breaking change

This unwraps nested refs on the template

NOTE: this needs to be intensively tested

@pikax pikax requested review from antfu and LinusBorg June 9, 2020 15:19
test/setup.spec.js Outdated Show resolved Hide resolved
src/reactivity/unwrap.ts Outdated Show resolved Hide resolved
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@antfu antfu mentioned this pull request Jun 9, 2020
@antfu
Copy link
Member

antfu commented Jun 10, 2020

LGTM. If you are ok with it, I can do the resolving and merge it now.

@pikax
Copy link
Member Author

pikax commented Jun 10, 2020

Sure :)

@antfu antfu merged commit 1fd48f5 into vuejs:master Jun 10, 2020
@pikax pikax deleted the fix/unwrap_nested_refs branch June 10, 2020 11:20
return value
}

if (isReactive(value)) {
Copy link
Contributor

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

https://github.com/vuejs/composition-api/pull/361/files#diff-ffb3fccc5cc98e58adb1477f88e460b83b048c4b693b3fdb479a559f0c66cb88R550-R552

asking because i'm running into a reproduction and for a minute i was tricked by a premature understanding of this check

Copy link
Member Author

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

Comment on lines +529 to +530
<p id="nested_aaaa_bbb_cc">{{ nested.aaaa.bbb.c }}</p>
<p id="nested_aaaa_bbb_cc">{{ nested.aaaa.bbb.cc }}</p>
Copy link
Contributor

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)

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants