fix(reactivity): ensure that shallow and normal proxies are tracked seperately (close #2843)

[LinusBorg]

This fixes #2843

We currently track reactive proxies in one WeakMap, not differentiating between normal and shallow Proxies. the same is true for readonly proxies.

This means that for each target, only one kind of proxy can be created, because we use these WeakMaps to re-use existing proxies for each known target.

const target = { some: 'value' }
const normalProxy = reactive(target)
const shallowProxy = shallowReactive(target)

expect(normalProxy).not.toBe(shallowProxy)

The above test fails - shallowProxy will in fact be the same proxy as normalProxy.

This PR adds separate WeakMaps to track shallow Proxies on their own, which makes the above test pass.

[github-actions]

Size report

Path	Size
vue.global.prod.js	41.29 KB (+0.07% 🔺)
runtime-dom.global.prod.js	27.39 KB (+0.16% 🔺)
size-check.global.prod.js	16.57 KB (+0.13% 🔺)

[LinusBorg]

Ah dang, it seems I had the sort-imports extension active involuntarily. That's why there a bunch of changes to the import statementsnthat are unnecessary.

Let me know wether I should revert those.

[jods4]

Aside: I haven't checked but I am pretty sure readonly works slightly differently?
At least you can wrap a reactive instance into a readonly one and get 2 different objects.
Not sure what happens if you call readonly on the target directly.

Simply the idea of wrapping one object in different reactive wrappers is asking for trouble. That issue should be resolved by "please, don't do this" and maybe make a PR to throw when someone attempts that in debug mode.

The "least" problem in my opinion that this assumes you have the raw target living somewhere + a reactive wrapper alive somewhere else and you attempts to create a 3rd instance that should be a shallow wrapper. That's 3 different instances for the same object, it's recipe for referential identity problem. The best pattern is to only keep only a single reactive proxy alive.

The worst problem is that you have no encapsulation and totally unpredictable code. If I create a shallowReactive, it's probably because I assume object X that I'm putting in this won't become reactive.
But if another part of my code uses the same object through a reactive proxy, then that contract can very easily be broken and that wouldn't be anything but a bug.

If your answer to this is: "no because my code is well organized and will never write back an object that was initially assigned by the shallow proxy"; then organize your code even better and split that object into 2 parts: one clearly deep, the other clearly shallow. Mix and matching based on instances won't lead to nice code.

Just the idea of creating proxies all over the place is wacky to me. Wrap your objects at creation time and your life will be much simpler.

Happy 2021 everyone!

[LinusBorg]

Aside: I haven't checked but I am pretty sure readonly works slightly differently?
At least you can wrap a reactive instance into a readonly one and get 2 different objects.
Not sure what happens if you call readonly on the target directly.

reactive proxies and readonly proxies are tracked in individual WeakMaps, so they don't influence each other. And that makes total sense of course, as you often might want to have a reactive proxy internally, i.e. in a composable function, and expose a readonly proxy publicly in its return value.

Simply the idea of wrapping one object in different reactive wrappers is asking for trouble.

I kind of agree with that sentiment, at least for reactive vs. shallowReactive(*)

The question you raise here is basically wether or not we should allow people to shoot themselves in the foot with these, or force them to find different routes t a solution.

Either way, I'd say the current implementation is to be rated a bug as a shallowReactive should definitely not return a deep reactive proxy without any kind of warning, and vice versa. Would you agree?

That issue should be resolved by "please, don't do this" and maybe make a PR to throw when someone attempts that in debug mode.

This PR does two things:

1. It enables us to actually differentiate between shallow and deep reactive proxies by tracking them in their own WeakMaps
2. It uses these different WeakMaps to allow fo both a deep and shallow reactive proxy to be created for the same target.

Whatever we do, we need 1. (or something like it) to actually be able to differentiate bewteen the two.

For 2., we could:

• implement a warning but still return the desired, "correct" proxy
• throw an error, essentially forbidding the creation of both a shallow and reactive proxy for the same target

However I'm not sure if we actually want to restrict usage in this way? shallow* proxies are already marked as dangerous/advanced features that can lead to identity hazards and similar problems in the docs, and if people follow the general principle of never touching their raw targets, they wouldn't come into this situation anyway.

So the question becomes: Are there valid use cases where people should be allowed to tread into this dangerous territory, or should we "protect" people from actually going there even if the purposefully want to?

---

[*]: readonly behaves differently, and I think the change I introduce here to allow both a readonly and shallowReadonly on the same target makes sense and actually fixes a bug.

[jods4]

Either way, I'd say the current implementation is to be rated a bug as a shallowReactive should definitely not return a deep reactive proxy without any kind of warning, and vice versa. Would you agree?

I agree. I would throw in debug mode if someone attempts that.
I'm not one to encourage closed-designs and I'm all for extensible frameworks.
But in this specific instance it just sounds like asking for trouble to me and a good framework should also lead its users away from potential problems.

Are you aware of use-cases where creating one shallow and one deep proxy would look like the good solution?

However I'm not sure if we actually want to restrict usage in this way?

If something appears later on, we can always relax the restriction or come up with a specific design to address the concrete use-case.
On the other hand, tightening would be a breaking change, so it's something that's best decided early on (i.e. now).

Whatever we do, we need 1. (or something like it)

Two "something like it" alternative ideas. Single WeakMap might be more efficient but would require being able to detect whether the proxy is shallow or not:

• Could be done by flagging the proxies with a symbol or something;
• or with one efficient WeakMap for all + a second "shallow" WeakMaps just to know if the proxy found in the first one is shallow or not. Assuming most proxies are deep, the 2nd would stay pretty empty.

Out of topic:

shallow* proxies are already marked as dangerous/advanced features that can lead to identity hazards and similar problems in the docs

This is an anecdote about my own usage, but I'm using lots and lots of shallow proxies/refs and the more I do so, the more I feel comforted in that choice.
Anyway with all the changes made to Vue since the betas, it's nice that you can use a deep or shallow style by default and both work well without having to fight the framework anymore. To each their own.

[lijingfaatcumt]

This fixes #2843

We currently track reactive proxies in one WeakMap, not differentiating between normal and shallow Proxies. the same is true for readonly proxies.

This means that for each target, only one kind of proxy can be created, because we use these WeakMaps to re-use existing proxies for each known target.

const target = { some: 'value' }
const normalProxy = reactive(target)
const shallowProxy = shallowReactive(target)

expect(normalProxy).not.toBe(shallowProxy)

The above test fails - shallowProxy will in fact be the same proxy as normalProxy.

This PR adds separate WeakMaps to track shallow Proxies on their own, which makes the above test pass.

i want to connect to you, but fail, because in you pr code, may cause shallowReactive(reactive(shallowReactive(reactive({}))))
infinitelty, may you modify you code?

[LinusBorg]

@lijingfaatcumt What behavior would you in that case?

[lijingfaatcumt]

@lijingfaatcumt What behavior would you in that case?

@LinusBorg in vue3, designing reactiveMap and readonlyMap is for cache the proxy version if the target has the proxy or the target is a proxy, but in you pr, the code below will cause the cache useless

let proxy = reactive(origin)
let shallowProxy = shallowProxy(origin)

let proxyCopy = reactive(shallowProxy)
proxyCopy === proxy // false

in code above, i want to make proxyCopy === proxy returns true
in order to solve the problem,the createReactiveObject function may modify follow below

function createReactiveObject(
  target: Target,
  isReadonly: boolean,
  baseHandlers: ProxyHandler<any>,
  collectionHandlers: ProxyHandler<any>,
  proxyMap: WeakMap<Target, any>
) {
  if (!isObject(target)) {
    if (__DEV__) {
      console.warn(`value cannot be made reactive: ${String(target)}`)
    }
    return target
  }
  // target already has corresponding Proxy
  const existingProxy = proxyMap.get(target)
  if (existingProxy) {
    return existingProxy
  }
  if (proxyMap.has(target)) {
    return target
  }
  // target is already a Proxy, return it.
  // exception: calling readonly() on a reactive object
  if (
    target[ReactiveFlags.RAW] &&
    !(isReadonly && target[ReactiveFlags.IS_REACTIVE])
  ) {
    //avoid duplication proxy
    target = toRaw(target)
    const existingProxy = proxyMap.get(target)
    if (existingProxy) {
      return existingProxy
    }
  }
  // only a whitelist of value types can be observed.
  const targetType = getTargetType(target)
  if (targetType === TargetType.INVALID) {
    return target
  }
  const proxy = new Proxy(
    target,
    targetType === TargetType.COLLECTION ? collectionHandlers : baseHandlers
  )
  proxyMap.set(target, proxy)
  return proxy
}

and i pull a pr to you

[LinusBorg]

Converted this back to a draft as there's both ongoing discussion about the approach itself by @jods4 and an edge case raised by @lijingfaatcumt that isn't covered yet.

[lijingfaatcumt]

Converted this back to a draft as there's both ongoing discussion about the approach itself by @jods4 and an edge case raised by @lijingfaatcumt that isn't covered yet.

@LinusBorg i have find the new way to solve the problem by #3052 ， it use 'isReadonly' and 'isShallow' property to avoid duplication proxy. the createReactiveObject function may modify follow below

function createReactiveObject(
  target: Target,
  isReadonly: boolean,
  shallow: boolean,
  baseHandlers: ProxyHandler<any>,
  collectionHandlers: ProxyHandler<any>
) {
  if (!isObject(target)) {
    if (__DEV__) {
      console.warn(`value cannot be made reactive: ${String(target)}`)
    }
    return target
  }
  const proxyMap = getProxyMap(isReadonly, shallow)
  // target already has corresponding Proxy
  const existingProxy = proxyMap.get(target)
  if (existingProxy) {
    return existingProxy
  }
  // target is already a Proxy, return it.
  // exception: calling readonly() on a reactive object
  if (target[ReactiveFlags.RAW]) {
    if (
      target[ReactiveFlags.IS_READONLY] === isReadonly &&
      target[ReactiveFlags.IS_SHALLOW] === shallow
    ) {
      return target
    }
    if (!isReadonly || !target[ReactiveFlags.IS_REACTIVE]) {
      target = toRaw(target)
      const existingProxy = proxyMap.get(target)
      if (existingProxy) {
        return existingProxy
      }
    }
  }
  // only a whitelist of value types can be observed.
  const targetType = getTargetType(target)
  if (targetType === TargetType.INVALID) {
    return target
  }
  const proxy = new Proxy(
    target,
    targetType === TargetType.COLLECTION ? collectionHandlers : baseHandlers
  )
  proxyMap.set(target, proxy)
  return proxy
}

[lijingfaatcumt]

Aside: I haven't checked but I am pretty sure readonly works slightly differently?
At least you can wrap a reactive instance into a readonly one and get 2 different objects.
Not sure what happens if you call readonly on the target directly.

Simply the idea of wrapping one object in different reactive wrappers is asking for trouble. That issue should be resolved by "please, don't do this" and maybe make a PR to throw when someone attempts that in debug mode.

The "least" problem in my opinion that this assumes you have the raw target living somewhere + a reactive wrapper alive somewhere else and you attempts to create a 3rd instance that should be a shallow wrapper. That's 3 different instances for the same object, it's recipe for referential identity problem. The best pattern is to only keep only a single reactive proxy alive.

The worst problem is that you have no encapsulation and totally unpredictable code. If I create a shallowReactive, it's probably because I assume object X that I'm putting in this won't become reactive.
But if another part of my code uses the same object through a reactive proxy, then that contract can very easily be broken and that wouldn't be anything but a bug.

If your answer to this is: "no because my code is well organized and will never write back an object that was initially assigned by the shallow proxy"; then organize your code even better and split that object into 2 parts: one clearly deep, the other clearly shallow. Mix and matching based on instances won't lead to nice code.

Just the idea of creating proxies all over the place is wacky to me. Wrap your objects at creation time and your life will be much simpler.

Happy 2021 everyone!

@jods4 i have solve duplication proxy by #3052, for the same origin object, reactive and shallowReactive is different proxy object, i think that is true, because reactive object and shallowReactive object has different baseHandlers

[jods4]

@lijingfaatcumt Not having 2 distinct instances for the same object is good, but I still have concerns with the concept of having both a deep and a shallow versions of the same object graph and all the unexpected behaviours it leads to.

It will unwillingly lead to having both raw and reactive versions of instances deeper in the graph.

[lijingfaatcumt]

@lijingfaatcumt Not having 2 distinct instances for the same object is good, but I still have concerns with the concept of having both a deep and a shallow versions of the same object graph and all the unexpected behaviours it leads to.

It will unwillingly lead to having both raw and reactive versions of instances deeper in the graph.

@jods4 in reactivity package, we call function reactive ,but may get a shallowReactive version or readonly version, this will make users confused. in my implements, the problems above will be solved. and this will make related reactive function more specific as the function's name show
In addition, I haven’t found the problem you mentioned. adding code of pr in #3007 mentioned, the solution i think is perfect.

in addition, the reactive and shallowReactive version have different operating types to the raw target, so i do not think this will be a problem if the reactive and shallowReactive will be stored different places

[jods4]

@lijingfaatcumt reactive returns a readonly version if its input is readonly and that's not something you can remove! I see you've changed the test for that in your PR but it's wrong.

If some code hands you out a readonly object, you should not get around it and start modifying it. If you do so you break the contract and most probably introduce bugs in the code that handed out the readonly object to you.

Take props from Vue itself. It's a readonly reactive object. What good do you think would happen if I start modifying it?

Now the next part is more arguable but to me it's the same thing with shallow and deep. If some part of my code created a shallow reactive object graph, it had a reason. If another part of my code takes the same object graph but then makes it deep, it's likely to have unintended consequences.

And in a nutshell that's why I think this PR is a bad idea.

[lijingfaatcumt]

@lijingfaatcumt reactive returns a readonly version if its input is readonly and that's not something you can remove! I see you've changed the test for that in your PR but it's wrong.

If some code hands you out a readonly object, you should not get around it and start modifying it. If you do so you break the contract and most probably introduce bugs in the code that handed out the readonly object to you.

Take props from Vue itself. It's a readonly reactive object. What good do you think would happen if I start modifying it?

Now the next part is more arguable but to me it's the same thing with shallow and deep. If some part of my code created a shallow reactive object graph, it had a reason. If another part of my code takes the same object graph but then makes it deep, it's likely to have unintended consequences.

And in a nutshell that's why I think this PR is a bad idea.

@jods4 first, i think readonly, reactive, shallowReactive only give you different types to operate the orgin objec. they are different proxy object. second we can pass arount the readonly by function toRaw and reactive and then operate the origin object, so the problem you said i pass around the readonly is has existing. this is not my fault. third, call reactive function may getting a readonly confused me. i think function's function should obviously to the function's name is important. in addition, reactive package is standalone, it's design should not affected to much by other packages in vue

[jods4]

second we can pass arount the readonly by function toRaw and reactive and then operate the origin object, so the problem you said i pass around the readonly is has existing

That's explicitly doing hacky stuff. If you really need / want to, it's fine.

The problem is that reactive is implicitly called on every object that is in a deep reactive object graph.

If a library hands out a readonly object to you, then you put it inside an object graph of your own, at no point should you get back a writable version.

// Let's say gatherStats() is a 3rd-party lib that gives you a read-only view of reactive stats.
let readOnlyStats = gatherStats();

// Let's say this is your component state and you put the stats in it
let myState = reactive({
  name: "Whatever",
  counters: readOnlyStats
});

// Now when you access stats in your model, they're writable?!
// No way this is good.
state.counters.x++;

call reactive function may getting a readonly confused me.

This might be because you misunderstood the contract of the function.
Reactive means that the object will notify when it changes. So you can use it in bindings or watch and they will magically update.
It does say nothing about whether you can write to it or not. readonly is a kind of reactive object.

[lijingfaatcumt]

second we can pass arount the readonly by function toRaw and reactive and then operate the origin object, so the problem you said i pass around the readonly is has existing

That's explicitly doing hacky stuff. If you really need / want to, it's fine.

The problem is that reactive is implicitly called on every object that is in a deep reactive object graph.

If a library hands out a readonly object to you, then you put it inside an object graph of your own, at no point should you get back a writable version.

// Let's say gatherStats() is a 3rd-party lib that gives you a read-only view of reactive stats.
let readOnlyStats = gatherStats();

// Let's say this is your component state and you put the stats in it
let myState = reactive({
  name: "Whatever",
  counters: readOnlyStats
});

// Now when you access stats in your model, they're writable?!
// No way this is good.
state.counters.x++;

call reactive function may getting a readonly confused me.

This might be because you misunderstood the contract of the function.
Reactive means that the object will notify when it changes. So you can use it in bindings or watch and they will magically update.
It does say nothing about whether you can write to it or not. readonly is a kind of reactive object.

@jods4 i undestand you, and i am wrong, i can not consider the nested proxy

[lijingfaatcumt]

second we can pass arount the readonly by function toRaw and reactive and then operate the origin object, so the problem you said i pass around the readonly is has existing

That's explicitly doing hacky stuff. If you really need / want to, it's fine.

The problem is that reactive is implicitly called on every object that is in a deep reactive object graph.

If a library hands out a readonly object to you, then you put it inside an object graph of your own, at no point should you get back a writable version.

// Let's say gatherStats() is a 3rd-party lib that gives you a read-only view of reactive stats.
let readOnlyStats = gatherStats();

// Let's say this is your component state and you put the stats in it
let myState = reactive({
  name: "Whatever",
  counters: readOnlyStats
});

// Now when you access stats in your model, they're writable?!
// No way this is good.
state.counters.x++;

call reactive function may getting a readonly confused me.

This might be because you misunderstood the contract of the function.
Reactive means that the object will notify when it changes. So you can use it in bindings or watch and they will magically update.
It does say nothing about whether you can write to it or not. readonly is a kind of reactive object.

@LinusBorg @jods4 i find a new way to solve your problems, and i pull a request to @LinusBorg