mobxjs / mobx-vue

🐉 Vue bindings for MobX
MIT License
475 stars 22 forks source link

TypeError: 'set' on proxy: trap returned falsish for property '__proto__' #15

Open lvvlan opened 6 years ago

lvvlan commented 6 years ago

Pls help, why is not working

This is demo https://codesandbox.io/s/lj3l95m09

Thx!

kuitos commented 6 years ago

The component which you used try to re-define the property of observable of mobx, use a non-mobx observable data or observable.ref instead.

class Model {
  @observable.ref
  data = []
}
lvvlan commented 6 years ago

Thx~ But if use @observable.ref, the changes in model do not respond to view,How should I do?

kuitos commented 6 years ago

do not modify the array with push or splice or such way, modify the reference instead. check the doc https://mobx.js.org/refguide/modifiers.html#reference-observability

lvvlan commented 6 years ago

Thx very much! But I don't understand what do u mean modify the reference instead, If I use @observable.ref, It means I can't change model?? This is simple demo https://codesandbox.io/s/lj3l95m09, that use @observable.ref, how should I change the model, it can be respond to view?

kuitos commented 6 years ago

What u decorated with observable.ref was val rather than the nested val.b.test, so re-assign val with a new reference then the reaction would be triggered.

handleChangeModel() {
    this.model.val.b.test = [
      {
        time: "我被改变了!"
      }
    ];
    this.model.val = {...this.model.val}
  }

To achieve the immutable approach u could try immer to reduce the verbose boilerplate code.

lvvlan commented 6 years ago

sorry sorry I'm wrong and update https://codesandbox.io/s/lj3l95m09 I think this.model= {...this.model}, u mean a new reference then the reaction would be triggered. however that mobx-vue is not used, because @Observer will replace Vue Watcher, right?

lvvlan commented 6 years ago

but this.model.val = {...this.model.val} can be update view, because I changed the reference, right?

kuitos commented 6 years ago

yep, observable.ref only react to reference changing

Nemikolh commented 5 years ago

Hi, I am also seeing a similar error after migrating vuejs from 2.5.17 to 2.5.18. However, I can't use observable.ref or pass { deep: false } to my observable as it breaks some of my observer. The codebase is relatively big, I will try to provide a minimal reproduction link.

Does that problem sounds familiar?

Nemikolh commented 5 years ago

Ok, so I managed to reproduce the problem: https://codesandbox.io/s/p7zwrvjrz7.

I'm really annoyed because it used to work until we had to update vuetify which in turns forced us to update vue to at least version 2.5.18. Maybe this bug should be reported on the vuejs repo?

EDIT: A simpler example that does not use vuetify: https://codesandbox.io/s/k90k8kwn2r

Note: To see the error select an item

Nemikolh commented 5 years ago

After investigating more this PR https://github.com/vuejs/vue/pull/7828 broke mobxjs/mobx-vue. @kuitos, I see several options:

Nemikolh commented 5 years ago

@kuitos: Could you re-open this issue until a solution is found?

kuitos commented 5 years ago

@Nemikolh Thanks for your great efforts, I will look into it this weekend:smiley:

Nemikolh commented 5 years ago

@kuitos Sounds awesome! I will let you know if I found other/better solutions.

bitencode commented 5 years ago

@kuitos @Nemikolh As far as I know this is still broken with any version of Vue > 2.5.17. I did some debugging and tracing over the weekend and added some more detail on the Vue issue referenced above. I kinda doubt that the Vue team will do anything about this because they want to hook into the Array prototype to make the array observable. I added what I found there. I can make the problem "go away", but I don't know what I'm breaking that I haven't found. I forked Mobx and added this check to the observable array set but feels risky:

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

This is in the code here: https://github.com/mobxjs/mobx/blob/daf6ac0ac8dd369fb6179ec6a7fbbb231f383f9f/src/types/observablearray.ts#L96-L109 that @Nemikolh pointed out. Adding that check makes our apps "work" in that exceptions aren't thrown and errors aren't logged - everything seems to work, but I don't know...

Do either of you have any thoughts on this? Should we open an issue on Mobx and see if there are any thoughts there? I'm a bit surprised this isn't a bigger issue - I thought more people were using Mobx with Vue.

Thanks!

mxs42 commented 5 years ago

Any update on this? I basically can't use any store that has arrays inside.

albertodvc commented 5 years ago

Same problem here. I'm building a POC to evaluate a possible migration of an old codebase to MobX/Vue but an issue like this being ignored for months...

kuitos commented 5 years ago

sorry for the delay.. could u pls provide a codesandbox reproduction for a better problem checking? @albertodvc

albertodvc commented 5 years ago

Thanks for the quick reply and sorry if my comment sounded a little rude. I will try to write one next week.

mxs42 commented 5 years ago

OK, after some debugging I discovered this. The core of this issue is the collectData function.

It filters data out for Vue, so if you pass MobX observable in Vue component's data (or as a class field if you use vue-class-component) it will check it and will not let Vue to perform its own observation. If isObservable returns false then it means that object isn't MobX observable and Vue will work to make it reactive.

In one of my Stores I had this to make TS correctly resolve type of 'playlists', I found it there:

export class PlaylistStore {
  playlists = observable<any>([]);
...
}

MobX' isObservable returns false for instances of this class! It means that if you add instance of this class as a field or data prop to one of Vue components, Vue will try to make it reactive! It sees that playlists is an array and it tries to replace its prototype to observe changes.

  if (Array.isArray(value)) {
    if (hasProto) {
      protoAugment(value, arrayMethods); // <- this line
    } else {
      copyAugment(value, arrayMethods, arrayKeys);
    }
    this.observeArray(value);
  } else {
    this.walk(value);
  }
function protoAugment (target, src) {
  /* eslint-disable no-proto */
  target.__proto__ = src;
  /* eslint-enable no-proto */
}

but MobX denies replacing of proto:

    set(target, name, value): boolean {
        if (name === "length") {
            target[$mobx].setArrayLength(value)
            return true
        }
        if (typeof name === "number") {
            arrayExtensions.set.call(target, name, value)
            return true
        }
        if (!isNaN(name)) {
            arrayExtensions.set.call(target, parseInt(name), value)
            return true
        }
        return false // <- returns false for `__proto__`
},

To sum up: in my case it was because some of my classes in fact were not observable by MobX. I will try to dig more into MobX internals but for my case it was because I didn't use @observable decorator in my class definition. You should avoid using hack by @bitencode! Although it works, it means that Vue treats your MobX observable objects wrong and it may impact further performance of your app because you essentially lose the connection between Vue and MobX for this object and your app will continue to work only because its Vue who will observe changes of this object.

TLDR: if you get this error, it means that you in fact use MobX and mobx-vue bindings wrong and your app isn't working correctly under the hood.

mxs42 commented 5 years ago

Speaking about this comment and reproduction link: this is wrong usage of mobx-vue.

When you write this

const store = observable({
  items: [{ id: "test", arr: [] }, { id: "foobar", arr: [] }]
});

you make store an observable but also store.items is an observable now and each item inside items is also an observable! console.log(isObservable(store.items[0])); // true

When you do this: <list @input="s => (selection = s)" :items="store.items"/> you assign an observable to selection that is being already observed by Vue (not mobx!) because you defined in inside data.

  data: () => ({
    store,
    selection: []
  })

Even toJS won't help you there because it will not convert s recursively since s isn't an observable, but s.arr is an observable and toJS stops when it sees first non-observable object.

The correct way to use above example is this. It seems that you have to use ViewModel instance to make a bridge between Vue and MobX, at least I didn't find an another way.

Obviously I'm not that familiar with internals of mobx and mobx-vue, maybe I assumed something wrong but it doesn't seem so judging by a code. Hope it helps to future readers.

albertodvc commented 5 years ago

Here is a sandbox with my case. It only fails on chrome. I have a collection store with a "contents" @observable array. When I keep a reference to the parent collection store in array items I got theTypeError: 'set' on proxy: trap returned falsish for property '__proto__'. when navigating to a single item view component. Remove the this.$store = store; line in Item constructor and Vue stop complaining.

mxs42 commented 5 years ago

@albertodvc it fails in every browser, it's not related to Chrome. Reason of failure is the same I wrote above, when you assign this.$store = store; your Item now has an observable inside but Item class itself isn't marked an "observable". Now when you assign item instance to data, Vue will try to observe it.

If you use ViewModel class example it will work correctly.

mxs42 commented 5 years ago

Another solution without a class is to wrap your data in observable

  data() {
    return {
      vm: observable({
        item: null
      })
    };
  },

I have not discovered better way to do it. I guess it's still possible though to make it better.

albertodvc commented 5 years ago

@mxs42 It works fine in Safari. Anyway I thought all array elements were observables when you decorated an array as observable.

mxs42 commented 5 years ago

@albertodvc oh I don't have Safari because I'm on Windows. It's not related to item being 'observable', it's related to the way Vue works. When you write this

  data() {
    return {
      item: null
    };
  },

you tell Vue to observe item. Now when you assign something to item Vue will observe it. If you assign Item instance with store as a property you tell Vue to observe Item and its fields. But store is already being observed by MobX, that's why it throws. Honestly I'm a bit frustrated because of the way Vue reactivity works, that's why we have such bugs and can not integrate another state management without such hacks.

albertodvc commented 5 years ago

Thanks for your time and explanation @mxs42. I think I could live with your wrapper solution, but there are a few things I still don't understand:

I'm starting to doubt MobX/Vue is a solid option to migrate my app. I have to go with Vue (corporate desition) It sounded nice we could rewrite our AngularJS state to mobx and start migrating component by component to Vue but now I'm afraid MobX integration with Vue could imply more and more hacks in the future.

mxs42 commented 5 years ago

@albertodvc

  1. It throws only once because after the first attempt to observe $store Vue adds __ob__ to $store prototype and because objects are passed by reference, it effectively modifies $store instance globally. Vue will not do observation setup again if object has __ob__ inside.

    /**
    * Attempt to create an observer instance for a value,
    * returns the new observer if successfully observed,
    * or the existing observer if the value already has one.
    */
    function observe (value, asRootData) {
    if (!isObject(value) || value instanceof VNode) {
    return
    }
    var ob;
    if (hasOwn(value, '__ob__') && value.__ob__ instanceof Observer) {
    ob = value.__ob__;
    } else if (
    shouldObserve &&
    !isServerRendering() &&
    (Array.isArray(value) || isPlainObject(value)) &&
    Object.isExtensible(value) &&
    !value._isVue
    ) {
    ob = new Observer(value); // <- if object doesn't have __ob__ then Vue creates Observer, and then Observer tries to replace __proto__ for arrays.
    }
    if (asRootData && ob) {
    ob.vmCount++;
    }
    return ob
    }
  2. It does throw in this case. Consider this example:

    
    import { observable } from "mobx";

class Test { @observable array = []; }

export default class Item { $id = null; $store = null; name = ""; description = "";

constructor(id, store) { this.$test = new Test(); this.$id = id; }

updateFromJson(json) { this.name = json.name; this.description = json.description; } }


now `$test` has an observable array and Vue will try to overwrite its prototype. It throws for me in Chrome and Firefox.

3. No idea about that, sorry. The only way to know is to step through debugger but I don't have any Safari near to me 😁

Sadly yes, I feel the same. I'm using MobX/Vue currently for new app but this situation made me think about possible alternatives. I can't stand Flux/Vuex because of all this boilerplate and triggering different actions by their names passed as strings. And I also don't like React, although it seems that MobX works better with React. 
MobX looks solid because it actually allows to use real classes and real methods and not this `store.commit('bla-bla-bla')`. It's possible that integration could be better but again, I don't know much about Vue and MobX internals so I can't tell exactly what can be improved.
kevlarr commented 5 years ago

@mxs42 @albertodvc Really appreciate the discussions here, and the similar observations. I enjoyed migrating an app to Vue until I added Vuex - similar complaints about typical flux/redux boilerplate, triggers, and string passing, and beyond that the complete inability to leverage Typescript was a deal-breaker.

I got excited again when I started converting Vuex to Mobx because of the clarity of the classes/stores (and the inherent Typescript support) - until lots of little edge cases like this and others pretty much showed that Vue's reactivity system won't play nicely with others to the point that I'm now considering migrating to Ember Octane or Svelte..

mxs42 commented 5 years ago

@kevlarr consider moving all state to MobX, i.e. if you need to compute something from store you must do it inside MobX viewmodel and then in your template you refer to instance of that viewmodel. Actually talking about your example in #11 — you can just reference computed Store.things in your template, right?

I currently continue to develop my app using MobX. Yes, it's not that amazing how I thought it is going to be but flux-like libraries is a big no-no for me. I wish Vue had better mechanism to use 3rd party state management though.

Also: I found that Habitica uses own implementation of Vuex without mutations. Might be a source for inspiration to create your own state management.

Svelte sounds great on paper but they got their own problems, the biggest one is that Svelte isn't that popular so you can't expect to see many 3rd party libraries and I personally find their syntax questionable.

Vue would be perfect if we had an ability to integrate MobX better but yea, our web-development world is far from perfect lol

mxs42 commented 5 years ago

I wrote comment above and realized again how fucked is a modern web development. We have to use thousands lines of code and another thousands lines to connect different lines of code to each other just to get things done. Many hacks and wrappers, complex build systems (and infinite amount of difficulties while trying to do something bigger than TodoMVC), total bullshit! I'm also a C++ developer and sometimes the whole situation makes me crazy because in web development it looks like I spend a lot of time just to setup something that is not even directly related to what I want to code. Like, I try to solve problems that were unnecessary and artificially created at the first place.

Sorry for offtopic, just needed to speak out lol

kevlarr commented 5 years ago

@mxs42 Yeah, agreed on Svelte - I re-read some of their docs and lost some interest, and same with Octane (I used to use Ember but disliked a lot of its patterns, and not enough is being 'fixed').. I'll check out how Habitica does state management with Vue, that sounds interesting! @kuitos mentioned moving all state to Mobx and I had not really thought of that as a good pattern before, but both of you make it sound pretty compelling now.

... realized again how fucked is a modern web development

Way too true haha. Off-topic or not, I enjoyed it! Makes me feel a lot more sane, too. Web front-end development is such a mire.

arenddeboer commented 5 years ago

Ahhh, I was so hoping Mobx-vue to be my replacement for vuex-module-decorators. But I've also hit this error. I'm really frustrated with TypeScript and Vue at the moment. Even considering a full rewrite to Angular even though I hate its enterprisy boilerplate and fast paced dev cycle.

mxs42 commented 5 years ago

Personally I finished small internal project without using any 3rd party state management library. I just created and used exported Vue instances as my stores, I think I recreated what Vuex does but without boilerplate and string actions. I mean I just did

export const sampleStore = new Vue({
    data() {
        return {
            foo: 'bar',
        },
    },
    methods: {
        doSomething() {

        }
    }
});

I didn't use TypeScript for this so it was bad in terms of autocompletion and IDE suggestions but I guess you can use typings for this. It worked and it's usable experience, now I consider using the same for other projects.

kevlarr commented 5 years ago

@mxs42 Nice, I think that's pretty similar to an email blast that Michael Thiessen sent out a week ago called "Why I don't use Vuex".. He was presenting an alternative of just using a Vue component as a store and injecting it where necessary (or just passing down via props)

Nemikolh commented 4 years ago

Hi @mxs42, I just wanted to thank you for coming up with a solution. I agree that the solution is not great. In my opinion the problem is that vue tries to do too much but YMMV. I guess for me what I want is a view layer closer to React / Preact. Anyway, thanks again!