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

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

Open
lvvlan opened this issue Jul 11, 2018 · 36 comments
Open

Comments

@lvvlan
Copy link

lvvlan commented Jul 11, 2018

Pls help, why is not working

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

Thx!

@kuitos
Copy link
Member

kuitos commented Jul 11, 2018

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

lvvlan commented Jul 11, 2018

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

@kuitos
Copy link
Member

kuitos commented Jul 12, 2018

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

lvvlan commented Jul 12, 2018

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

kuitos commented Jul 12, 2018

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

lvvlan commented Jul 12, 2018

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

lvvlan commented Jul 12, 2018

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

@lvvlan lvvlan closed this as completed Jul 12, 2018
@kuitos
Copy link
Member

kuitos commented Jul 12, 2018

yep, observable.ref only react to reference changing

@Nemikolh
Copy link

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

Nemikolh commented Jan 11, 2019

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

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

  • Submit a PR to github.com/mobxjs/mobx to allow setting __proto__ in some way. Not sure of the implication of the change.
  • Submit a PR to github.com/vuejs/vue to skip object of type Proxy.
  • Do something in this library? However I'm not sure if there's anything that can be done...

@Nemikolh
Copy link

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

@kuitos kuitos reopened this Jan 14, 2019
@kuitos
Copy link
Member

kuitos commented Jan 14, 2019

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

@Nemikolh
Copy link

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

@bitencode
Copy link

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

mxs42 commented May 6, 2019

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

@albertodvc
Copy link

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

kuitos commented May 9, 2019

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

@albertodvc
Copy link

albertodvc commented May 9, 2019

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

@mxs42
Copy link

mxs42 commented May 13, 2019

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

mxs42 commented May 13, 2019

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

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

mxs42 commented May 13, 2019

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

mxs42 commented May 13, 2019

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

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

@mxs42
Copy link

mxs42 commented May 13, 2019

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

albertodvc commented May 14, 2019

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:

  • Why it only throws once? If you go back to item list and go to any item view again, everything works but as far as I know a new instance of the route component is being created every time you navigate to a new route.
  • If I get you right the problem arise if Item instance has the store property which is being observed by MobX because store has observable properties: contents and status, so it should throw too if Item has an observable property different than store or an object property with observables inside... but it doesn't. (Maybe the reference cycle has something to do with this?)
  • Again I don't understand why is not throwing in Safari.

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

mxs42 commented May 14, 2019

@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
}
  1. 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.

  1. 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
Copy link

kevlarr commented May 19, 2019

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

mxs42 commented May 19, 2019

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

mxs42 commented May 19, 2019

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

kevlarr commented May 20, 2019

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

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

mxs42 commented Jul 3, 2019

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

kevlarr commented Jul 5, 2019

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

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!

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

No branches or pull requests

8 participants