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

Vue-Mobx integration broken with PR #7828 #9329

Open
bitencode opened this issue Jan 16, 2019 · 4 comments
Open

Vue-Mobx integration broken with PR #7828 #9329

bitencode opened this issue Jan 16, 2019 · 4 comments

Comments

@bitencode
Copy link

bitencode commented Jan 16, 2019

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: #7828.

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

@Nemikolh
Copy link

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
Copy link
Author

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
Copy link
Member

posva commented Jan 16, 2019

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

@bitencode
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants