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

feat(reactivity): proxyRefs method and ShallowUnwrapRefs type #1682

Merged
merged 3 commits into from
Jul 28, 2020

Conversation

yyx990803
Copy link
Member

@yyx990803 yyx990803 commented Jul 22, 2020

BREAKING CHANGE

Template auto ref unwrapping for setup() return object is now applied only to the root level refs.

Rationale

This change aims to ensure that non-ref values referenced in templates retain the same identity with the value declared inside setup() regardless of whether it's wrapped with reactive or readonly.

Currently, the object returned from setup() is exposed to the template render context as a reactive proxy. This means all properties accessed from the template, even nested ones, will be reactive proxies as well. This has led to a edge cases that can be confusing:

<div v-for="item in list">{{ isInList(item) }}</div>
setup() {
  const list = [
    { foo: 1 },
    { bar: 2 }
  ]

  function isInList(item) {
    // item here is a proxy of the original!
    return list.includes(item)
  }

  return {
    list,
    isInList
  }
}

live demo

Before this change, the render result will be false false because the item accessed in the template are proxies and not strictly equal to the original items declared in setup().

Another case involves class instances exposed to templates:

{{ foo.getValue() }}
class Foo {
  count = ref(1)
  getValue() {
    // `this` here is a proxy of the raw instance
    return this.count.value
  }
}

// ...
setup() {
  return {
    foo: new Foo()
  }
}

live demo

Here, this would point to the proxy in foo.getValue because the foo in the template is a proxied version of the class instance, and this results in this.count being an already unwrapped number, and this.count.value will be undefined (hence the render reuslt will be blank). In other words, the behavior of the class becomes inconsistent based on whether it's accessed through the template or not.

We have went back-and-forth on this behavior during the alpha/beta phases. Previously, we felt we could encourage users to always explicitly wrap/mark returned objects with markRaw, reactive or readonly to avoid such problems, but it always feel like a potential footgun. Although we indicated that we are trying to avoid breaking changes during the RC phase, we feel this it is important to have this discussed before the final release.

Breakage Details

The breaking case is that given the following:

setup() {
  return {
    rootRef: ref(1),
    object: {
      nestedRef: ref(3)
    }
  }
}

After this change, {{ rootRef }} in the template will still render 1, but {{ object.nestedRef }} will no longer be auto unwrapped as 3.

Note that if an object is a reactive or readonly proxy, they still come with deep ref unwrapping by themselves. This change only affects plain objects containing refs.

Composition function returning plain object with refs

A common case where this could happen is returning an object of refs from a composition function:

function useFeatureFoo() {
  const someValue = ref(0)
  const otherValue = ref(1)

  // composition function returning a plain object
  // intended to facilitate destructuring in setup()
  return {
    someValue,
    otherValue
  }
}

export default {
  setup() {
    return {
      // however the consuming component wants to use it as a nested object
      // and access features as {{ foo.someValue }} in the template
      foo: useFeatureFoo()
    }
  }
}

The fix is rather straightforward: wrap it with reactive to unwrap the refs:

setup() {
  return {
    foo: reactive(useFeatureFoo())
  }
}

Another aspect of this is that users probably do this to avoid the trouble of destructuring and then returning the setup object, which can become tedious if there are many properties in the returned object:

setup() {
  const { someValue, otherValue, ... } = useFeatureFoo()
  return {
    someValue,
    otherValue,
    ...
  }
}

which can be simplified in <script setup> as:

<script setup>
export const { someValue, otherValue, ... } = useFeatureFoo()
</script>

Setup returned properties are no longer reactive

Another minor breakage is that this also makes non-ref properties in the returned objects non-reactive:

setup() {
  return {
    count: 0
  }
}

Mutating count from the template, e.g. with <button @click="count++"> will no longer trigger updates.

/cc @jods4 @RobbinBaauw @basvanmeurs

BREAKING CHANGE: template auto ref unwrapping are now applied shallowly,
i.e. only at the root level.

  This change aims to ensure that non-ref values referenced in templates
  retain the same identity with the value declared inside `setup()`
  regardless of whether it's wrapped with `reactive` or `readonly`.

  The breaking case is that given the following:

  ```js
  setup() {
    return {
      one: ref(1),
      two: {
        three: ref(3)
      },
      four: 4
    }
  }
  ```

  After this change, `{{ one }}` in the template will still render `1`,
  but `{{ two.three }}` will no longer be auto unwrapped as `3`.

  A common case where this could happen is returning an object of refs
  from a composition function. The recommendation is to either expose
  the refs directly, or call the newly exposed `proxyRefs` on the object
  so that the object's refs are unwrapped like root level refs. In fact,
  `proxyRefs` is also the method used on the `setup()` return object.

  This also makes non-ref properties in the returned objects non-reactive,
  so mutating `four` from the template will no longer trigger updates.
@@ -561,7 +561,7 @@ export function handleSetupResult(
}
// setup returned bindings.
// assuming a render function compiled from template is present.
instance.setupState = reactive(setupResult)
instance.setupState = proxyRefs(setupResult)
Copy link
Contributor

Choose a reason for hiding this comment

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

@yyx990803 is it on purpose that proxyRefs doesn't check if its argument is reactive and always wraps it with a new proxy?
I very often return state that is reactive({}) and it seems wasteful to always go 2 layers deep in proxies to access state, when just one would be perfectly fine.

I'd rather have proxyRefs be a default fallback when I pass a plain object.

Or going full circle to one of my very first suggestions: not wrapping at all and leaving the responsibility of picking a wrapper to setup. (it seems reasonable that <script setup> always wraps the exports in a proxyRefs).

@jods4
Copy link
Contributor

jods4 commented Jul 22, 2020

We have went back-and-forth on this behavior during the alpha/beta phases.

Yup this was discussed at great lengths during the alpha, so I'm a bit surprised by the late change!

I think we established that with proxies there will always be unfortunate edge cases but there were 2 "safer" patterns:

  • make everything (deeply) a proxy, which is what we went with;
  • keep everything shallow by default, which was not favored if I recall correctly because it wasn't ergonomic enough, especially considering auto-unref'ing was a must-have.

This change feels a bit like a middle ground to me, which is arguably the worst option because you get all the drawbacks of every other option.

  1. This change is targeted at people who have non-proxy objects in their graph (something we said we would discourage).
    If you're gonna use raw objects, this is just a band-aid that fixes the first level in templating. Because reactive primitives are deep by default, including both ref and reactive, it's still pretty easy to shoot yourself in the foot and mix a raw object into a deep proxy. E.g. just consider a plain list of items and a ref that holds a currently selected item. Very common scenario and you end up with both a raw and a proxy version of the selected item.

  2. We loose on the ergonomics of the deep proxies. The must-have unref'ing is now only available at first-level or must be explicitly opted into through some reactive object.

I won't be one to complain about this change as it rather goes into the direction I preferred, but it doesn't feel totally "right". This change supports patterns that deviate from the previous recommendation, which makes me wonder: what is the new recommended pattern that is generally safe?

@yyx990803
Copy link
Member Author

yyx990803 commented Jul 23, 2020

Auto un-ref is mostly an ergonomic need at the root level, because it is only at the root level you are likely to get a mix of plain objects, reactive objects and refs.

  • For root level refs, the behavior is exactly the same as before.
  • For reactive objects, the behavior is also exactly the same as before. These are the entry points to the "reactive graphs" and they are still deeply proxied.

This change is targeted at people who have non-proxy objects in their graph

No. This change really only affects a single case: plain objects at the root level - which most of the time is intentional, and only this case leads to the confusing behavior discussed in the rationale. This change fixes exactly those cases.

We loose on the ergonomics of the deep proxies.

As long as you expose reactives you still get deep proxies. We do lose a bit of convenience when choosing not to destructure composition function return objects, but I feel we can work around it by encouraging users to expose refs at the root - which I think is less footgun-ish than encouraging explicit markRaw or readonly. When you forget to call proxyRefs on a plain object with refs you'll notice immediately that it's not working, but with the latter you only notice in very specific cases that are hard to reason about for non-experts.

That said, the motivation is to fix the confusing cases mentioned above, which are much more confusing than "ref unwrapping only works at root level" - the latter is a rather straightforward rule to remember.

@RobbinBaauw
Copy link
Contributor

RobbinBaauw commented Jul 23, 2020

Wow!

This looks great, but it may be possible to improve it even more: I made an implementation of something similar as an experiment, and the nested auto unref also worked there. It was done by also unreffing in the component proxy (RobbinBaauw@d4653a9#diff-cc6f02998e28ee54ec621c0be5c7b5caL230).

This means that the last level of refs is unwrapped, irrespective of the depth. This however has the downside that if you make the nested object a ref, you'll lose the auto unreffing (if it's for example done on the second layer, the first will still see auto unref):

setup()​ ​{​
  ​return​ ​{​
    ​rootRef​: ​ref(1),​
    ​object​: ​{​
      ​nestedRef​: ​ref(3)​
    ​}​
  ​}​
​}

will work, whereas

setup()​ ​{​
  ​return​ ​{​
    ​rootRef​: ​ref(1),​
    ​object​: ​{​
      ​nestedObjectRef​: ​ref({
          nestedRef: ref(3)
      })​
    ​}​
  ​}​
​}

will not, as the nested object is not unreffed.

So on the one side this is an improvement, but on the other side it creates new edge cases that would have to be explained (even though an object ref will probably not be common)

@yyx990803
Copy link
Member Author

yyx990803 commented Jul 23, 2020

@RobbinBaauw I may be mistaken but your approach doesn't seem to do deep unwrap (i.e. object.nestedRef will still be a ref in templates). The unref is also only called for root level properties in setupState. Also unwrapping in instance proxies won't account for direct field access.

@RobbinBaauw
Copy link
Contributor

@yyx990803 I believe that with the proxyRefs and the unref in the component proxy, the "first and last" layers of a nested object are unreffed. This is because everything returned from the component proxy can just be unreffed.

I don't have access to a pc currently though, so I'm not 100% sure, however iirc something similar to your example worked on that branch (once I get home I'll verify this).

Even though I think this covers many cases, the "only the first layer is unreffed" may be more friendly to end users as there is a clear line as to what is automatically unreffed and what is not.

@basvanmeurs
Copy link
Contributor

  1. This change is targeted at people who have non-proxy objects in their graph (something we said we would discourage).

Why is this a serious problem that must be resolved?

We have an application in which we use a datamodel (classes with properties and methods) and tend to provide these in the setup context, which can then be used. But when the this scope becomes a proxy may lead to serious performance problems when non-raw objects are being used.

There are two possible solutions from a user perspective:

  1. use markRaw
  2. do not provide these things to the setup context, and provide computedRefs that provide the values instead

To be honest, we mix both workarounds right now.

But solution 1 is really easy to forget. You may experience strange errors as described by @RobbinBaauw that take you a couple of days to understand.. But there is a far worse implication: doing it wrong only once destroys all JS engine optimizations without noticing it (see example).

Solution 2 can be quite tedious. It leads to far more verbose code than just adding a .value to refs :-)

This change feels a bit like a middle ground to me, which is arguably the worst option because you get all the drawbacks of every other option.

To me it feels quite the opposite. It is the best of both worlds:

  1. Users are no longer at risk of destroying the JS optimizations by simply adding a non-raw object to the setup context (or experiencing the errors)
  2. You can still omit .value for setup vars. Is it unlogical that this is not recursive? Not to me.. actually initially I was surprised in the first place to find reactivity/refs being recursive.

So I am really happy with this PR. But it could be even better.

Just drop the component proxy and always reference the setup raw object as _ctx in render functions. You won't be able to reach the app context and data etc, but who cares? You can always add them in your setup object when you need them (or parts of it). I know there's backwards compatibility, but I never used vue2 so personally I don't care about it.. What I do care about is the performance (and readability) gains, and those are major. A flag to toggle this behavior would be great. We need to look forward as well. I would happily implement this in a PR if you'd consider this flag/functionality, but would rather not waste the time if that's not the case ;-)

Comment on lines +180 to +182
export type ShallowUnwrapRef<T> = {
[K in keyof T]: T[K] extends Ref<infer V> ? V : T[K]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is better ?

export type ShallowUnwrapRef<T> = T extends ReturnType<typeof reactive>
  ? T
  : { [K in keyof T]: T[K] extends Ref<infer V> ? V : T[K] }

@yyx990803 yyx990803 merged commit aa06b10 into master Jul 28, 2020
@yyx990803 yyx990803 deleted the setup-unwrap branch July 28, 2020 20:30
yangmingshan added a commit to vue-mini/vue-mini that referenced this pull request Jul 29, 2020
pikax added a commit to pikax/vue-function-api that referenced this pull request Aug 4, 2020
BREAKING CHANGE: template auto ref unwrapping are now applied shallowly,
i.e. only at the root level. See vuejs/core#1682 for
more details.
pikax added a commit to vuejs/composition-api that referenced this pull request Aug 6, 2020
* feat: `proxyRefs` method and `ShallowUnwrapRefs` type

BREAKING CHANGE: template auto ref unwrapping are now applied shallowly,
i.e. only at the root level. See vuejs/core#1682 for
more details.
@rchl
Copy link

rchl commented Sep 2, 2020

After this or some later changes I'm seeing weird behavior with my refs being unwrapped in watchers. For example:

<template>
  <div>Check the console</div>
</template>

<script>
import { reactive, watchEffect, ref } from "@vue/composition-api";

export default {
  setup() {
    const temp = ref("xxx");

    const state = {
      value: ref(0)
    };

    watchEffect(() => {
      console.log("state.value:", state.value);
      temp.value = "";
    });

    return {
      state: reactive(state)
    };
  }
};
</script>

When you open the console you'll see that the first time the watchEffect is triggered the state.value is a ref (RefImpl). The second time it's just an unwrapped number.

It only happens when returning "reactive" state to the template.

Here is live demo: https://codesandbox.io/s/lucid-hooks-y48qt?file=/src/App.vue:0-408

@pikax
Copy link
Member

pikax commented Sep 2, 2020

@rchl this is v3 repo and your issue is with the composition api plugin.

Vue2 reactivity system mutates the original object, so when you do reactive will change 6our state object and unwrap it, this as nothing to do with the template.

@rchl
Copy link

rchl commented Sep 2, 2020

My bad, will report there.

lisadeloach63 added a commit to lisadeloach63/composition-api-Php-laravel that referenced this pull request Oct 7, 2022
* feat: `proxyRefs` method and `ShallowUnwrapRefs` type

BREAKING CHANGE: template auto ref unwrapping are now applied shallowly,
i.e. only at the root level. See vuejs/core#1682 for
more details.
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

7 participants