Vue-Mobx integration broken with PR #7828

bitencode commented 5 years ago

Version

2.5.18

Reproduction link

https://codesandbox.io/s/k90k8kwn2r

Steps to reproduce

Use the mobxjs/mobx-vue package (or a custom integration with Mobx)
Everything worked fine on Vue 2.5.17 and is broken on 2.5.18 and above
See that TypeErrors are thrown

What is expected?

Vue would not attempt to to set the __proto__ property of a Proxy.

What is actually happening?

Reactivity with Mobx is broken:

[Vue warn]: Error in event handler for "input": "TypeError: 'set' on proxy: trap returned falsish for property 'proto'"

We attempted to upgrade Vue from 2.5.17 to latest version for our applications - thought it would be smooth because upgrade target was still 2.5.x, but ever version after 2.5.18 is broken with Mobx integration.

We have a custom integration with Mobx-state-tree, but some of the code is very similar to mobxjs/mobx-vue integration code. We are not using that library, and I'm not a Vue internals expert, but I think I tracked the problem down to this PR: https://github.com/vuejs/vue/pull/7828.

Since then I have searched around and found this issue reported to that package also: https://github.com/mobxjs/mobx-vue/issues/15#issuecomment-453570397. @Nemikolh wrote the codesandbox example I referenced above and it appears to be the same conclusion.

Nemikolh commented 5 years ago

Thanks for mentioning me, the other option that I think would be doable is to change mobx itself so that it would allow setting __proto__ for an array. The problematic code is in arrayTraps.set function:

https://github.com/mobxjs/mobx/blob/daf6ac0ac8dd369fb6179ec6a7fbbb231f383f9f/src/types/observablearray.ts#L96-L109

I'm not sure of the impact of such a change though.

Either way, it would be nice to have test(s) that are run as part of the test suite to verify the mobx integration. I think they should go into the mobx-vue repo but they would need to be triggered whenever a new minor version of vue and/or mobx is released. That might not be trivial to do.

bitencode commented 5 years ago

Good point @Nemikolh - I have no context for which might be the better place to fix this. I followed the Vue path because that's the package that appeared to break. Maybe core Vue and/or Mobx contributors will have an opinion (that is compatible😜). It does seem that tests would be well placed in mobx-vue.

posva commented 5 years ago

what is the actual thing breaking Vue, without any mobx/mobx-vue import?

bitencode commented 5 years ago

Hi @posva, to be honest, probably nothing; but neither is anything broken in Mobx unless you try to use Vue - so I'm a bit at a loss as to how to proceed 😞 With my personal apps (using Vue and Vuex) nothing is wrong. When I'm working with my employers app (Vue, Mobx, MST) this function:

function protoAugment (target, src: Object) {
  /* eslint-disable no-proto */
  target.__proto__ = src
  /* eslint-enable no-proto */

in vue/src/core/observer/index.js invokes the Mobx set on observable arrays and returns false (causing the vue warning). https://github.com/mobxjs/mobx/blob/daf6ac0ac8dd369fb6179ec6a7fbbb231f383f9f/src/types/observablearray.ts#L96-L109

I forked the Mobx repo and added a check to the end of the set function:

        if (name === "__proto__") {
            return true
        }
        return false

But I have a really bad feeling about that as I have no idea what I might be breaking in Vue. It looks like Vue is trying to hook into the prototype chain to observe changes, but Mobx is already doing that. So, I don't know what the answer is.

vuejs / vue