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

observed showed up at the prototype chain of original #1246

Closed
guaijie opened this issue May 27, 2020 · 5 comments
Closed

observed showed up at the prototype chain of original #1246

guaijie opened this issue May 27, 2020 · 5 comments

Comments

@guaijie
Copy link
Contributor

guaijie commented May 27, 2020

Version

3.0.0-beta.14

Reproduction link

[http:// forge.com](http:// forge.com)

Steps to reproduce

test.only('prototype', () => {
    const observed = reactive({ foo: 1 })
    const original = Object.create(observed)
    let dummy
    effect(() => (dummy = original.foo))
    expect(dummy).toBe(1)
    observed.foo = 2
    expect(dummy).toBe(2)
    original.foo = 3
    expect(dummy).toBe(3) // ①
    original.foo = 4
    expect(dummy).toBe(3) // ②
  })

If assertion ① is true, then assertion ② is obviously not the expected result.
This can be confusing for developers because the two assertions contradict each other.

Maybe the result is more appropriate, because we should simply regard the first assignment as an operation on original, which is just a original object

original.foo = 3
expect(dummy).toBe(2) // ①
original.foo = 4
expect(dummy).toBe(2) // ②

Because of the toraw method, the condition judgment in the following code will become meaningless

    const hadKey = hasOwn(target, key)
    const result = Reflect.set(target, key, value, receiver)
// don't trigger if target is something up in the prototype chain of original
    if (target === toRaw(receiver)) {
      if (!hadKey) {
        trigger(target, TriggerOpTypes.ADD, key, value)
      } else if (hasChanged(value, oldValue)) {
        trigger(target, TriggerOpTypes.SET, key, value, oldValue)
      }
    }

Maybe it's more appropriate

// don't trigger if target is something up in the prototype chain of original
    if (target === toRaw(receiver)) {
       const hadKey = hasOwn(target, key)
       const result = Reflect.set(target, key, value, receiver)
      if (!hadKey) {
        trigger(target, TriggerOpTypes.ADD, key, value)
      } else if (hasChanged(value, oldValue)) {
        trigger(target, TriggerOpTypes.SET, key, value, oldValue)
      }
    }

Rewrite toraw method (do not get the raw of observed on prototype chain)

The operation of original object should not have side effects(The operation of original will not be to modify observed.foo , and the 'trigger' method is not triggered.)

What is expected?

The operation of original object should not have side effects

What is actually happening?

The change of the original object has side effects, which may lead to unintended results

@guaijie
Copy link
Contributor Author

guaijie commented May 28, 2020

it.only('should observe inherited property accessors', () => {
    let dummy, parentDummy, hiddenValue: any
    const obj = reactive<{ prop?: number }>({})
    const parent = reactive({
      set prop(value) {
        hiddenValue = value
      },
      get prop() {
        return hiddenValue
      }
    })
    Object.setPrototypeOf(obj, parent)
    effect(() => (dummy = obj.prop))
    effect(() => (parentDummy = parent.prop))

    expect(dummy).toBe(undefined)
    expect(parentDummy).toBe(undefined)
    obj.prop = 4
    expect(dummy).toBe(4)
    // this doesn't work, should it?
    // expect(parentDummy).toBe(4) // ①
    parent.prop = 2
    expect(dummy).toBe(2) // ②
    expect(parentDummy).toBe(2) 
  })

I don't think the assertion ① should be tenable. If it‘s tenable, something will become complicated and unpredictable.
In the same way, assertion ② should not be tenable. It is interlinked with assertion ①'s internal logic. In fact, it is also interlinked with my view put forward above

function createGetter(isReadonly = false, shallow = false) {
  return function get(target: object, key: string | symbol, receiver: object) {
    if (key === ReactiveFlags.isReactive) {
      return !isReadonly
    } else if (key === ReactiveFlags.isReadonly) {
      return isReadonly
    } else if (key === ReactiveFlags.raw) {
      return target
    }

    const targetIsArray = isArray(target)
    if (targetIsArray && hasOwn(arrayInstrumentations, key)) {
      return Reflect.get(arrayInstrumentations, key, receiver)
    }
    const res = Reflect.get(target, key, receiver)

    if (isSymbol(key) && builtInSymbols.has(key) || key === '__proto__') {
      return res
    }

    if (shallow) {
      !isReadonly && track(target, TrackOpTypes.GET, key)
      return res
    }

    if (isRef(res)) {
      if (targetIsArray) {
        !isReadonly && track(target, TrackOpTypes.GET, key)
        return res
      } else {
        // ref unwrapping, only for Objects, not for Arrays.
        return res.value
      }
    }

    !isReadonly && track(target, TrackOpTypes.GET, key)
    return isObject(res)
      ? isReadonly
        ? // need to lazy access readonly and reactive here to avoid
          // circular dependency
          readonly(res)
        : reactive(res)
      : res
  }
}

Whether the execution of 'track' should also add judgment conditions like the execution of 'trigger' in the setter

if (target === toRaw(receiver)) {
       !isReadonly && track(target, TrackOpTypes.GET, key)
}

This makes it clear to developers that they should not rely on observed on the prototype chain to get reactive data.
It will avoid these unpredictable results

@zhangenming
Copy link
Contributor

调试响应式代码好头大哦
一层一层的 都是各种代理getter 一会脑袋就凌乱了

@yyx990803 yyx990803 reopened this Jun 11, 2020
@yyx990803
Copy link
Member

Accidentally closed by wrong issue tag in commit message -.-

@guaijie
Copy link
Contributor Author

guaijie commented Jul 3, 2020

Accidentally closed by wrong issue tag in commit message -.-

???

@guaijie
Copy link
Contributor Author

guaijie commented Jul 3, 2020

the above problems have not been resolved

@github-actions github-actions bot locked and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants