mobxjs / mobx-vue

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

Using mobx computed inside vue computed #11

Open xrado opened 6 years ago

xrado commented 6 years ago

When using mobx computed inside vue computed, vue computed don't get updated.

https://codesandbox.io/s/m5k1yrn2xx click add

kuitos commented 6 years ago

That is because mobx-vue do not modify/wrap the original vue computed definition, so the mobx observable is not recognized by vue thus mobx changes will not trigger vue data reevaluation.

BTW, we suggest using mobx to manage all your state rather than mixed them in, since you had chosen mobx🙂.

xrado commented 6 years ago

I agree, it is probably better to move all the state to mobx, but sometimes it comes handy if you can mix mobx shared state with vue local state. Is this vue computed modification/wrapping even possible, as you better know the internals?

kuitos commented 6 years ago

yes it is possible and not very complicated. But if we convert vue computed into mobx computed as the first step, it means we should take over the whole vue watcher system, means we should not only convert computed but also convert vue props to observable, vue watch to observe, vue method to action, and so on. And, the most important is, all of these conversion are implicit, the syntactic inconsistencies between vue and mobx will make us confused and may introduce more development bugs.

Local states are useful but we could also construct them with mobx. As a workaround you could do like this:

class ViewModel {}

const App = observer({
  data: { vm: new ViewModel() }
})

Thanks for your feedback!😀

kevlarr commented 5 years ago

@kuitos How would that example let one use Vue and mobx computed properties together? We are running into this issue and lacking a good solution for making something like...

// A global singleton
import Store from '@/store';

@Observer
@Component
class SomeComponet extends Vue {
    @computed
    get things() {
        return Store.things; // A computed property
    }
}

... into a reactive property is basically preventing us from adopting mobx in our Vue apps.

kevlarr commented 5 years ago

At the very least, it would be nice if this inability to use mobx computed inside of Vue computed is called out in the main README as a more obvious limitation or this issue is left in an "opened" state to make it easier to find. If there won't be a fix, this should at least be surfaced more.

gmoneh commented 5 years ago

I’ll second this request, and would suggest to be reconsidered. There are plenty of use cases where a computed mobx and computed vue property together would be really useful.

kuitos commented 5 years ago

reopen this issue, try to find a solvable way

gmoneh commented 5 years ago

Thanks for reconsidering this. I don't think this has to extend to all the other Vue mechanisms. There is a clear difference for example between MobX actions and Vue methods. The latter can invoke the former if necessary, no need to interlope them. Similar with watches... that's equivalent to a MobX reaction and no need to merge those... one works for local state and other for the observable. Although some decorator to be able to add "reaction" methods in a Vue component would be another great improvement. The other improvement I can see, which might or might not be related to the computed properties, is the firing of the Updated hook when the component is re-rendered because of changes in the observable state.

partyka1 commented 5 years ago

Probably it could be achieved with writing @computed decorator in a way that it registers all accessed properties as dependency (Dep.target.depend()) in Vue dependencies/watchers system.

partyka1 commented 5 years ago

Simplest workaround is to disable cache on that getter:

//decorators.js
import { createDecorator } from 'vue-class-component'

export const NoCache = createDecorator((options, key) => {
  // component options should be passed to the callback
  // and update for the options object affect the component
  options.computed[key].cache = false
})

//MyComp.vue
@Component
class MyComp extends Vue {
  // the computed property will not be cached
  @NoCache
  get random () {
    return Math.random()
  }
}

read more: https://github.com/vuejs/vue-class-component#create-custom-decorators

partyka1 commented 5 years ago

Ive written universal @computed decorator, that can be used on vue classes, and as a mobx computed decorator:

import {createDecorator} from 'vue-class-component';
import Vue from 'vue';
import {computed as mobxComputed, IComputed} from 'mobx';

/**
 * Disables cache on vue-class-component getter
 */
export const NoCache = createDecorator((options: any, key: string) => {
    // component options should be passed to the callback
    // and update for the options object affect the component
    options.computed[key].cache = false;
});

/**
 * Enables to use mobx observables in vue-class-component getters
 * @param target
 * @param key
 * @param index
 */
export const computed: IComputed | any = (target: any, key: any, index: any) => {
    // Using Vue's computed on MobX's computed is not supported: https://github.com/mobxjs/mobx-vue/issues/11
    // adding mobx's @computed --> results in `call stack exceeded`
    // In order to use MobX observables in Vue computed, cache must be disabled on this computed:
    if (target instanceof Vue) {
        return NoCache(target, key, index);
    } else {
        return mobxComputed(target, key, index);
    }
};
jbbch commented 4 years ago

Any updates on the issue?

partyka1 commented 4 years ago

@jbbch for now you can just use the solution i posted. If it's only for aliasing MobX store variables purposes, you won't have any performance drawbacks. If you have some calculations inside that getter you should move it to MobX getter and end result will be the same as you would use cache provided by Vue computed properties

jbbch commented 4 years ago

@partyka1 Thanks for the proposition! I specifically want to keep MobX store properties and Vue components' representation properties separated. By doing so, I need to have calculations inside my getters and can't use your solution.

partyka1 commented 4 years ago

i think its not stressed enough, but assumption of this plugin is when you install it you ditch vue watch system in favour of mobx. you shouldnt mix vue computed and mobx store. thats the assumption and design of mobx-vue, so probably this issue wont be followed by fix from the dev team.

partyka1 commented 4 years ago

i think it should be one of the section in the readme about this problem just so people know what they are getting into :)

partyka1 commented 4 years ago

Also, I think showing warning with short description of the issue in development environment if someone uses @computed on vue-class-component getter. @kuitos what's your take on this?