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): avoid duplication proxy #3052

Closed
wants to merge 7 commits into from
Closed

fix(reactivity): avoid duplication proxy #3052

wants to merge 7 commits into from

Conversation

lijingfaatcumt
Copy link

@lijingfaatcumt lijingfaatcumt commented Jan 19, 2021

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,
  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
}

@posva
Copy link
Member

posva commented Jan 19, 2021

As said in the other PR, please provide a reproduction or failing test when reporting or fixing a bug Nevermind, the colors on github branch names confused me 🙃

@LinusBorg
Copy link
Member

You have a point here, but I don't think the solution is the best one.

Checking the WeakMap for an existing entry is a comparatively expensive OP, which is why it's been placed after the check of ReactiveFlags.RAW etc.

Will look further into it later.

@lijingfaatcumt
Copy link
Author

lijingfaatcumt commented Jan 19, 2021

You have a point here, but I don't think the solution is the best one.

Checking the WeakMap for an existing entry is a comparatively expensive OP, which is why it's been placed after the check of ReactiveFlags.RAW etc.

Will look further into it later.

@LinusBorg i push the pr again, modify the solution of avoiding duplication proxy by 'isReadonly' and 'isShallow' property

@yyx990803 yyx990803 force-pushed the linusborg/map-shallow-proxies-seperately-2843 branch from 694cdc0 to 4b79bac Compare March 26, 2021 19:35
@yyx990803 yyx990803 closed this Mar 26, 2021
@yyx990803 yyx990803 deleted the branch vuejs:linusborg/map-shallow-proxies-seperately-2843 March 26, 2021 19:39
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.

4 participants