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

Using reactive and shallowReactive on the same target returns the same proxy for both calls. #2843

Closed
lijingfaatcumt opened this issue Dec 18, 2020 · 10 comments · Fixed by #2851
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. 🐞 bug Something isn't working

Comments

@lijingfaatcumt
Copy link

lijingfaatcumt commented Dec 18, 2020

Version

3.0.4

Reproduction link

https://jsfiddle.net/vqkho2xt/2/

Steps to reproduce

const origin = {};
const shallowProxy = shallowReactive(origin);
const reactiveProxy = reactive(origin);
console.log(shallowProxy === reactiveProxy);

What is expected?

shallowProxy !== reactiveProxy

What is actually happening?

shallowProxy === reactiveProxy


why reactive and shallowReactive should return diffrent object ?
may there is the situation: we maybe want to get two different Proxy with the same origin object, the proxy named reactiveProxy returned by reactive , the proxy named shallowProxy returned by shallowReactive, we change the origin objects deep property by reactiveProxy can trigger denpendencies of the deep propery, but we change the origin objects deep property by shallowProxy should not trigger denpendencies of the deep propery, in this situation, the implements of the vue3 can not satisfy because of that shallowReactive and reactive return the same proxy with the same origin object.

@edison1105
Copy link
Member

IMO, this will make shallowProxy and reactiveProxy confused. Developers do not know how to choose which API to use.

@lijingfaatcumt
Copy link
Author

IMO, this will make shallowProxy and reactiveProxy confused. Developers do not know how to choose which API to use.

I do not think so, because i think 'shallow' can show the difference, i think we should use 4 weakMap (reactiveMap, shallowReactiveMap, readolyMap, shallowReadonlyMap) to store the map relationship of the origin object to the proxy instead of the 2 maps(reactiveMap and readonlyMap)

@posva posva added 🐞 bug Something isn't working 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. labels Dec 18, 2020
@posva
Copy link
Member

posva commented Dec 18, 2020

I think that changing the order of creation of shallowReactive and reactive shouldn't change the outcome of this: https://jsfiddle.net/u8ga1pcb/

@LinusBorg
Copy link
Member

LinusBorg commented Dec 18, 2020

We collect all reactive instances in one WeakMap, that ignores what type of reactive they are:

https://github.com/vuejs/vue-next/blob/b81d279e40c3d32a3740ea20d7e182690294f117/packages/reactivity/src/reactive.ts#L22-L30

This likely affects readonly vs. shallowReadonly as well. Meaning we would in fact need 4 Weakmaps to track each type individually

@lijingfaatcumt
Copy link
Author

We collect all reactive instances in one WeakMap, that ignores what type of reactive they are:

https://github.com/vuejs/vue-next/blob/b81d279e40c3d32a3740ea20d7e182690294f117/packages/reactivity/src/reactive.ts#L22-L30

This likely affects readonly vs. shallowReadonly as well. Meaning we would in fact need 4 Weakmaps to track each type individually

the actions of reactive and shallowReactive are different? why we ignore the difference?

@lijingfaatcumt
Copy link
Author

lijingfaatcumt commented Dec 18, 2020

We collect all reactive instances in one WeakMap, that ignores what type of reactive they are:
https://github.com/vuejs/vue-next/blob/b81d279e40c3d32a3740ea20d7e182690294f117/packages/reactivity/src/reactive.ts#L22-L30

This likely affects readonly vs. shallowReadonly as well. Meaning we would in fact need 4 Weakmaps to track each type individually

the actions of reactive and shallowReactive are different? why we ignore the difference?

i have the origin object, first i reactive origin and get reactive, second i shallowReactive the origin, but i get the reactive, can not get the shallowReactive, and i do not know the result is the reactive, this is confused,because of this, cause the bug #2846

@LinusBorg
Copy link
Member

i have the origin object, first i reactive origin and get reactive, second i shallowReactive the origin, but i get the reactive, can not get the shallowReactive, and i do not know the result is the reactive, this is confused,because of this, cause the bug

yes. This is why we marked it as a bug.

I will close your othr issues as a duplicate

@LinusBorg
Copy link
Member

PR submitted, happy to get reviews.

@LinusBorg LinusBorg changed the title how can i get diffrent proxy with reactive and shallowReactive functions Using reactive and shallowReactive on the same target returns the same proxy for both. Dec 19, 2020
@LinusBorg LinusBorg changed the title Using reactive and shallowReactive on the same target returns the same proxy for both. Using reactive and shallowReactive on the same target returns the same proxy for both calls. Dec 19, 2020
@edison1105
Copy link
Member

@lijingfaatcumt

Insert code like this in markdown.
It will enhance the readability of the code

image

@lijingfaatcumt
Copy link
Author

lijingfaatcumt commented Jan 12, 2021

PR submitted, happy to get reviews.

@LinusBorg in you code of pr, i found some improve, i think should modify below

export function reactive(target: object) {
  return createReactiveObject(
    target,
    false,
    mutableHandlers,
    mutableCollectionHandlers,
    reactiveMap
  )
}

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
  let existingProxy = proxyMap.get(target)
  if (existingProxy) {
    return existingProxy
  }
 // target is already a Proxy, should toRaw prevent new proxy again
  // exception: calling readonly() on a reactive object
  if (
    target[ReactiveFlags.RAW] &&
    !(isReadonly && target[ReactiveFlags.IS_REACTIVE])
  ) {
    target = toRaw(target)
    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
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. 🐞 bug Something isn't working
Projects
None yet
4 participants