Skip to content

Commit

Permalink
refactor(runtime-core): revert setup() result reactive conversion
Browse files Browse the repository at this point in the history 
BREAKING CHANGE: revert setup() result reactive conversion

    Revert 6b10f0c & a840e7d. The motivation of the original change was
    avoiding unnecessary deep conversions, but that can be achieved by
    explicitly marking values non-reactive via `markNonReactive`.

    Removing the reactive conversion behavior leads to an usability
    issue in that plain objects containing refs (which is what most
    composition functions will return), when exposed as a nested
    property from `setup()`, will not unwrap the refs in templates. This
    goes against the "no .value in template" intuition and the only
    workaround requires users to manually wrap it again with `reactive()`.

    So in this commit we are reverting to the previous behavior where
    objects returned from `setup()` are implicitly wrapped with
    `reactive()` for deep ref unwrapping.
  • Loading branch information
@yyx990803
yyx990803 committed Feb 27, 2020
1 parent 11d2fb2 commit e67f655
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 46 deletions.
7 changes: 2 additions & 5 deletions packages/runtime-core/src/component.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { VNode, VNodeChild, isVNode } from './vnode'
import {
reactive,
ReactiveEffect,
shallowReadonly,
pauseTracking,
Expand Down Expand Up @@ -398,7 +399,7 @@ export function handleSetupResult(
}
// setup returned bindings.
// assuming a render function compiled from template is present.
instance.renderContext = setupResult
instance.renderContext = reactive(setupResult)
} else if (__DEV__ && setupResult !== undefined) {
warn(
`setup() should return an object. Received: ${
Expand Down Expand Up @@ -474,10 +475,6 @@ function finishComponentSetup(
currentInstance = null
currentSuspense = null
}

if (instance.renderContext === EMPTY_OBJ) {
instance.renderContext = {}
}
}

// used to identify a setup context proxy
Expand Down
37 changes: 6 additions & 31 deletions packages/runtime-core/src/componentProxy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,7 @@ import {
ComputedOptions,
MethodOptions
} from './apiOptions'
import {
ReactiveEffect,
isRef,
isReactive,
Ref,
ComputedRef,
unref
} from '@vue/reactivity'
import { ReactiveEffect, UnwrapRef } from '@vue/reactivity'
import { warn } from './warning'
import { Slots } from './componentSlots'
import {
Expand Down Expand Up @@ -48,17 +41,11 @@ export type ComponentPublicInstance<
$nextTick: typeof nextTick
$watch: typeof instanceWatch
} & P &
UnwrapSetupBindings<B> &
UnwrapRef<B> &
D &
ExtractComputedReturns<C> &
M

type UnwrapSetupBindings<B> = { [K in keyof B]: UnwrapBinding<B[K]> }

type UnwrapBinding<B> = B extends ComputedRef<any>
? B extends ComputedRef<infer V> ? V : B
: B extends Ref<infer V> ? V : B

const publicPropertiesMap: Record<
string,
(i: ComponentInternalInstance) => any
Expand Down Expand Up @@ -115,17 +102,17 @@ export const PublicInstanceProxyHandlers: ProxyHandler<any> = {
case AccessTypes.DATA:
return data[key]
case AccessTypes.CONTEXT:
return unref(renderContext[key])
return renderContext[key]
case AccessTypes.PROPS:
return propsProxy![key]
// default: just fallthrough
}
} else if (data !== EMPTY_OBJ && hasOwn(data, key)) {
accessCache![key] = AccessTypes.DATA
return data[key]
} else if (hasOwn(renderContext, key)) {
} else if (renderContext !== EMPTY_OBJ && hasOwn(renderContext, key)) {
accessCache![key] = AccessTypes.CONTEXT
return unref(renderContext[key])
return renderContext[key]
} else if (type.props != null) {
// only cache other properties when instance has declared (this stable)
// props
Expand Down Expand Up @@ -180,19 +167,7 @@ export const PublicInstanceProxyHandlers: ProxyHandler<any> = {
if (data !== EMPTY_OBJ && hasOwn(data, key)) {
data[key] = value
} else if (hasOwn(renderContext, key)) {
// context is already reactive (user returned reactive object from setup())
// just set directly
if (isReactive(renderContext)) {
renderContext[key] = value
} else {
// handle potential ref set
const oldValue = renderContext[key]
if (isRef(oldValue) && !isRef(value)) {
oldValue.value = value
} else {
renderContext[key] = value
}
}
renderContext[key] = value
} else if (key[0] === '$' && key.slice(1) in target) {
__DEV__ &&
warn(
Expand Down
13 changes: 3 additions & 10 deletions test-dts/defineComponent.test-d.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import {
defineComponent,
PropType,
ref,
Ref,
reactive,
createApp
} from './index'
Expand Down Expand Up @@ -65,15 +64,12 @@ describe('with object props', () => {
// setup context
return {
c: ref(1),
d: reactive({
d: {
e: ref('hi')
}),
},
f: reactive({
g: ref('hello' as GT)
}),
h: {
i: ref('hi')
}
})
}
},
render() {
Expand Down Expand Up @@ -106,9 +102,6 @@ describe('with object props', () => {
expectType<string>(this.d.e)
expectType<GT>(this.f.g)

// should not unwrap refs nested under non-reactive objects
expectType<Ref<string>>(this.h.i)

// setup context properties should be mutable
this.c = 2

Expand Down

12 comments on commit e67f655

@yyx990803
Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @jods4

@jods4
Copy link
Contributor

@jods4 jods4 commented on e67f655 Feb 27, 2020

Choose a reason for hiding this comment

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

oh, I'm disappointed. 😞
Makes me call markNonReactive on every setup() method. On the other hand, you have to call react yourself anyway on most stuff otherwise it wouldn't be reactive in your event handlers, watches, etc.

Is refs in deep objects graphs a common thing? Doesn't feel that way to me.
Refs are an annoyance that is required to move plain values around. If you have an object, it's just easier to make the object reactive than put refs in it.
And if you go the deep refs way, you can still call reactive yourself.

I'm also afraid that at one point the non-intuitive behavior of unwrapping/not unwrapping stuff based on different contexts is gonna create confusion.
Unwrapping the first level is a bit magic, but the pattern of passing a bunch of named refs from setup is common enough to warrant it.

Having a built-in reactive at the root is gonna make everything reactive for most users that don't have a clue how it works internally.

  • That's gonna hurt perf when they bind big graphs (google "large list performance vue").
  • It creates additional proxies that will cause unexpected issues to people using sets/maps/strict equality in their templates and code.

Of course, it is possible to work around it both ways: with markNonReactive or reactive calls, depending on what the framework chooses to be the default behavior.
I just think the less magical behavior is more intuitive, easier to explain, performs better and causes less unexpected issues.

@yyx990803
Copy link
Member Author

@yyx990803 yyx990803 commented on e67f655 Feb 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is refs in deep objects graphs a common thing?

Refs are going to be very common in composition functions (functions that abstract reusable logic). Most of them will be returning a plain object containing refs (either created via a literal, or by calling toRefs on a reactive object).

The pitfall of no conversion is that, given a useFoo() function that returns { bar: ref(1), baz: ref(2) }, the user can either go:

setup() {
  const { x, y } = useFoo() // foo is of shape { bar: ref(1), baz: ref(2) }
  return {
    x,
    y,
    // ...other properties
  }
}

Or:

setup() {
  const foo = useFoo() // foo is of shape { bar: ref(1), baz: ref(2) }
  return {
    foo,
    // ...other properties
  }
}

Without implicit deep unwrapping, the user would have to do foo.bar.value inside the template in the 2nd case, which can be unexpected.

Honestly, I don't know why you'd need markNonReactive in every setup() - based on my experience (given that data() in Vue has always performed deep conversion by default), the cases where explicit non-reactiveness is needed are quite rare. You only return what needs to be used in a template, and if something is used in the template, it most likely should be reactive, otherwise the re-rendering won't function properly.

That's gonna hurt perf when they bind big graphs

Vue 3's reactive conversion is lazy, so as long as there is pagination it will only convert objects as they are accessed. Without the implicit reactivity, I'd imagine Vue 2 users getting confused when a deep mutation inside an Array doesn't trigger updates as expected. Either way, the basic intuition in Vue has always been that "everything is reactive by default, unless explicitly marked otherwise".

The unwrapping is indeed magic, but I believe the "magic-ness" is well worth the usability improvements in template authoring.

@jods4
Copy link
Contributor

@jods4 jods4 commented on e67f655 Feb 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'd rather encourage useFoo to return a reactive { bar: number, baz: number }.
Template aside, that's easier to consume in code.

I know you're gonna say the pitfall is doing return { bar: foo.bar } because it's just a value and you've lost reactivity.
And I agree that it is an unfortunate pitfall, but at one point a basic understanding of reactivity is gonna be required.

if something is used in the template, it most likely should be reactive

In practice, I find that a lot of data is non-reactive.
I'm very conscious about it because I'm an oddball and have the opposite approach to "everything is reactive by default, unless explicitly marked otherwise".

Without the implicit reactivity, I'd imagine Vue 2 users getting confused when a deep mutation inside an Array doesn't trigger updates as expected.

Except you're solving unwrapping, not reactivity. If a user holds a non-reactive array or object inside setup() and mutates it, it won't update in UI.

Another source of confusion: it unwraps as long as it's refs inside objects, not inside arrays (a recent change).

It also creates object identity confusion with === and friends. Example:

const Component = {
  setup() {
    const dogs = [ { id: 1, name: 'milou' }, { id: 2, name: 'rintintin' } ]
    function clicked(dog) {
      dogs.includes(dog) // false??
    }
    return { dogs, clicked }
  },

  template: `<button v-for='d of dogs' @click='clicked(d)>`
}

I don't care too strongly about this because there's an escape hatch that I can use, but I feel like there is a lot of corner cases that can create confusion.

Explaining that in Vue 2 data is the boundary for reactive state; and in Vue 3 reactive() calls are, might be easier.

@pikax
Copy link
Member

@pikax pikax commented on e67f655 Feb 28, 2020

Choose a reason for hiding this comment

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

I'd rather encourage useFoo to return a reactive { bar: number, baz: number }.
Template aside, that's easier to consume in code.

It makes it much harder to compose multiple uses, because if you have a useBaz(useFoo().baz) it will not be reactive, because by extracting baz from a reactive without toRefs will make it a plain object.

if you return { bar: Ref<number>, baz: Ref<number> } you can still keep reactivity using useBaz(useFoo().baz)

@jacekkarczmarczyk
Copy link
Contributor

Choose a reason for hiding this comment

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

It makes it much harder to compose multiple uses, because if you have a useBaz(useFoo().baz) it will not be reactive

You can use useBaz(useFoo()) and access it with arg.baz instead of baz.value, not saying that it's ideal solution but imho it's far from being "much harder"

@pikax
Copy link
Member

@pikax pikax commented on e67f655 Feb 28, 2020

Choose a reason for hiding this comment

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

You can use useBaz(useFoo()) and access it with arg.baz instead of baz.value, not saying that it's ideal solution but imho it's far from being "much harder"

That doesn't sound really extensive, you binding the arguments for useBaz to be dependent of useFoo, or at least to resemble the {baz: number}, IMO I don't think that's a good approach.

IMO the strengths of using composition API is creating and using building blocks

if you have other method:

function useFetchOptions(endpoint: string){
   const state = reactive({
    url : ref(endpoint),
    definitions: {
      baz: 0,
      // etc...
    }
  });
   // fetch stuff and populate state
  return state
}

useBaz(useFetchOptions('random.com').definitions.baz); // not reactive

// to make it reactive
const fetchOptions = useFetchOptions('')
const baz = computed(()=> fetchOptions.definitions.baz); // keep reactivity
useBaz(baz);

how would you reuse useBaz in this scenario?

Using ref

function useFetchOptions(endpoint: string){
   const baz = ref(0)
   // fetch stuff and populate baz
  return {
    url: endpoint,
    definitions: {
      baz
    }
  }
}

useBaz(useFetchOptions('random.com').definitions.baz); // still reactive

IMHO ref is the clear winner if you want to compose with other composable use

An actual implementation of fetching rest API (SWAPI) and adding pagination to the result
FullExample

function useSWAPI(r) {
  const resource = isRef(r) ? r : ref(r);
  const ENDPOINT = `https://swapi.co/api/`;

  const items = ref([]);
  const { json, loading, exec, status } = useFetch();

  const pagination = usePagination({
    currentPage: 1,
    pageSize: 10 // default size
  });

  watch(
    json,
    json => {
      pagination.total.value = json.count;

      items.value = json.results;
    },
    {
      lazy: true
    }
  );

  watch([pagination.currentPage, resource], () => {
    exec(`${ENDPOINT}${resource.value}/?page=` + pagination.currentPage.value);
  });

  return {
    ...pagination,
    exec,
    items,
    loading,
    status
  };
}

useSWAPI("people")

@jods4
Copy link
Contributor

@jods4 jods4 commented on e67f655 Feb 28, 2020

Choose a reason for hiding this comment

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

how would you reuse useBaz in this scenario?

Vue has a function for that purpose: toRefs
let { baz } = toRefs(definitions)
Then baz is a ref that proxies the definitions.baz and that you can pass around.
(toRefs could be enhanced to be lazy but that's another topic)

Fundamentally this discussion is not specific to plugins/mixins. It's inherent to working with observable/reactive data in general.
Stretching your argument to its extreme: we shouldn't have reactive at all because we are better served by using ref exclusively everywhere.

An actual implementation of fetching rest API (SWAPI) and adding pagination to the result

Nice example.

I think you have a bug in your json watch and it shows how it's hard to do all the .value correctly without help from typing.
If json is a ref, you should do json.value.count and json.value.results.
If json is an object with refs, you should do json.count.value and json.results.value.
If json is a reactive object, it doesn't work as the first watch argument should be an array, a ref or a function.

Here's a variant of your code. It's subjective but I like it better, much less .value all around.
Assume everything returns reactive objects.

const ENDPOINT = `https://swapi.co/api/`;

function useSWAPI(resource) {
  const fetch = useFetch();
  const pagination = usePagination({ currentPage: 1, pageSize: 10 });

  const data = reactive({
    items: [],
    ...toRefs(pagination),
    ...toRefs(fetch),
  });

  watchEffect(() => {
    if (!fetch.json) return; // assuming json is null until the network promise settles
    pagination.total = fetch.json.count;
    data.items = fetch.json.results;
  });

  watchEffect(() => fetch.exec(`${ENDPOINT}${unref(resource)}/?page=${pagination.currentPage}`));

  return data;
}

useSWAPI("people")

@pikax
Copy link
Member

@pikax pikax commented on e67f655 Feb 28, 2020

Choose a reason for hiding this comment

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

Vue has a function for that purpose: toRefs
let { baz } = toRefs(definitions)
Then baz is a ref that proxies the definitions.baz and that you can pass around.
(toRefs could be enhanced to be lazy but that's another topic)

I personally don't like to toRefs, and that's because I mainly return an object of ref. I think we can agree there's advantages and disadvantages of using either way.
I think it will come down as a preference.

Stretching your argument to its extreme: we shouldn't have reactive at all because we are better served by using ref exclusively everywhere.

I don't want to be extreme, they have their own usage case, personally sending ref as separate arguments conveys the fact that those arguments are meant to be reactive. Creating an reactive "state" in the function and then return toRefs, personally I think is a pretty clever idea and makes the code clean, but personally I never used it.

If json is a ref, you should do json.value.count and json.value.results.
If json is an object with refs, you should do json.count.value and json.results.value.
If json is a reactive object, it doesn't work as the first watch argument should be an array, a ref or a function.

  • json closure comes from the watch callback, that's unwrapped, so no need to do .value
  • json comes from useFetch and is just a await response.json()
  • json from setup() is ref<T> or in this case ref<any>

You can check the live example on CodeSandbox

Here's a variant of your code. It's subjective but I like it better, much less .value all around.
Assume everything returns reactive objects.

Looks cleaner. But you don't hide props from either usePagination or useFetch on that example, you just return everything, and using the spread ...toRefs properties might be overridden without anyone notice(but that has nothing to do with ref/reactive).

@jods4
Copy link
Contributor

@jods4 jods4 commented on e67f655 Feb 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I personally don't like to toRefs,

To be fair: I don't like it much, either. I see its utility as discussed above but I don't find the api compelling.
I'm trying to come up with something slightly different for my own projects, not 100% sure what yet.

json closure comes from the watch callback, that's unwrapped, so no need to do .value [...]

Right! I skimmed over the fact that there were 2 different json.
And I think it illustrates the point: it quickly gets confusing what is a ref and what is not (and this is just a really small example).
I've written similar code for years and it has constantly be the one thing that I wish could be improved in a reactivity lib.

But you don't hide props from either usePagination or useFetch on that example

It's really easy to do, though.
There are several ways to go about it, so let's get fancy. Here's a slightly different toRefs that is easy to write and still typesafe in TS:

const data = reactive({
    items: [],
    ...toRefs(pagination, 'total', 'currentPage'),
    ...toRefs(fetch, 'loading', 'status'),
  });

All our examples are far from perfect public apis. Returning refs means consuming code can modify its value, which is wrong.
Let's fix this with another solution:

const data = {
  // Just doing a handful for brievety
  get total() { return pagination.total },
  get currentPage() { return pagination.currentPage },
}

Bonus: it's also more efficient than all our previous examples.

And because that's a bit verbose you could write a different api to create that object:

const data = merge(
  getter(items, 'items'), // let's pretend it's a ref, as we only have a single variable of our own in this example
  getters(pagination, 'total', 'currentPage'),
  getters(fetch, 'loading', 'status'),
);

It's not hard to write those merge, getter, getters functions. Once you have them it's really clean to compose a new public state out of several reactive elements (both refs and reactives).

@jods4
Copy link
Contributor

@jods4 jods4 commented on e67f655 Mar 6, 2020

Choose a reason for hiding this comment

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

@yyx990803 How do you feel about this example:
https://codesandbox.io/s/elegant-jepsen-ydunm
When you select one of the three standard colors, it says It is not a standard color.
The root cause of that behavior is this revert here.

Do you think it's intuitive/ok?

@jods4
Copy link
Contributor

@jods4 jods4 commented on e67f655 Mar 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Playing w/ alpha.8 I noticed this, which might be expected but is worth noting:
The reactive wrapper replaces the proxy that unwrapped 1st level refs in alpha.7.
In other words, if you do this:

setup() {
  return markNonReactive({ x: ref(0) })
}

You are not getting back the same behavior as alpha.7.
In alpha 8 your x is only unwrapped magically by reactive and you opted out of it, so x is a ref inside template.

To get back alpha.7 behavior you need to write a new wrapper yourself, something like:

const handlers = {
  get(target, key, receiver) {
    return unref(Reflect.get(...arguments));
  }
}

function vm(data) {
  return markNonReactive(new Proxy(data, handlers));
}

// In your code:
setup() {
  return vm({ x: ref(0) });
}

EDIT:
But that doesn't work with ref in your template (I mean those refs: <div ref='el'>).
I guess Vue doesn't see the refs in the data object anymore (of course not). Internally I guess it must be peeping them before adding the wrapper.
(see also #660)
Right now, I'm really having a bad time upgrading to alpha 8. ☹️

EDIT 2:
Dug further down. Vue does not peep at the properties before wrapping.
When setting a ref string, the runtime calls toRaw on the context before looking for the ref.
This is a dead end for my solution above, because toRaw is based on the weakmap reactiveToRaw, which is not exposed publicly.
Which links to vuejs/rfcs#129

Please sign in to comment.