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

Using mobx computed inside vue computed #11

Open
xrado opened this issue Jun 21, 2018 · 17 comments
Open

Using mobx computed inside vue computed #11

xrado opened this issue Jun 21, 2018 · 17 comments
Labels
wontfix This will not be worked on

Comments

@xrado
Copy link

xrado commented Jun 21, 2018

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

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

@kuitos
Copy link
Member

kuitos commented Jun 21, 2018

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

xrado commented Jun 21, 2018

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

kuitos commented Jun 22, 2018

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!😀

@kuitos kuitos added the wontfix This will not be worked on label Jun 24, 2018
@kuitos kuitos closed this as completed Jun 27, 2018
@kevlarr
Copy link

kevlarr commented May 18, 2019

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

kevlarr commented May 18, 2019

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

gmoneh commented May 22, 2019

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 kuitos reopened this May 22, 2019
@kuitos
Copy link
Member

kuitos commented May 22, 2019

reopen this issue, try to find a solvable way

@gmoneh
Copy link

gmoneh commented May 22, 2019

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

partyka1 commented Sep 9, 2019

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

partyka1 commented Sep 9, 2019

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

partyka1 commented Sep 9, 2019

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

jbbch commented Oct 5, 2019

Any updates on the issue?

@partyka1
Copy link

partyka1 commented Oct 5, 2019

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

jbbch commented Oct 5, 2019

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

partyka1 commented Oct 5, 2019

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

partyka1 commented Oct 5, 2019

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

partyka1 commented Oct 5, 2019

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

6 participants