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

views don't update on vuex state change #416

Closed
terion-name opened this Issue Oct 23, 2016 · 16 comments

Comments

Projects
None yet
7 participants
@terion-name

terion-name commented Oct 23, 2016

I have a store:

//store/index.js
import Vue from 'vue'
import Vuex from 'vuex'
import constructor from './modules/constructor'

Vue.use(Vuex);

const debug = process.env.NODE_ENV !== 'production';

export default new Vuex.Store({
    modules: {
        constructor
    },
    strict: debug
})
//store/modules/constructor.js
export default {
    state: { // initial state
        slides: []
    },
    mutations: {
        ['constructor.addSlide'](state, type) {
            const slides = state.slides;
            slides.push({
                id: Math.round(Math.random() * 10000),
                type,
            });
            Vue.set(state, 'slides', slides);
        },
        ['constructor.updateSlide'](state, slide) {
            const slides = state.slides;
            const targetSlide = slides.find(s => s.id === slide.id);
            if (targetSlide) targetSlide.data = slide.data;
            //Vue.set(state, 'slides', {...slides});
            state.slides = Object.assign({}, slides);
        }
    }
}

I've tried different variations guided by http://vuex.vuejs.org/en/mutations.html#mutations-follow-vues-reactivity-rules

On the page I've got two almost similar components slides-previews and presentation-editor:

<div class="presentation-construtor">
    <slides-previews :slides="$store.state.constructor.slides"></slides-previews>
    <presentation-editor :slides="$store.state.constructor.slides"></presentation-editor>
    <templates-list @sidebar-toggle="toggleSidebar"></templates-list>
  </div>

<presentation-editor>:

<div class="slide-editor">
    <div class="viewport">
      <div class="wrap">
        <div class="slide-holder" v-for="slide in slides">
          <div class="slide-container">
            <component :is="slide.type" :data="slide.data" :id="slide.id"></component>
          </div>
        </div>
      </div>
    </div>
  </div>

and currently one slide type:

<template>
  <div class="slide presentation-slide">
    <div class="label">Идея</div>
    <div class="column" v-for="(value, index) in columns">
      <text-editor :content="value.title" mode="inline" @change="setTitle(index, ...arguments)"></text-editor>
      <text-editor :content="value.body" mode="partial" @change="setBody(index, ...arguments)"></text-editor>
    </div>
    <div class="controls">
      <button v-if="data.columns.length < 2" @click="addColumn">+</button>
    </div>
  </div>
</template>

<script>
  export default{
    props: {
      id: {},
      data: {
        default: () => {
          return {
            columns: [
              {
                title: 'Заголовок',
                body: 'Контент'
              }
            ]
          }
        }
      }
    },
    components: {
      textEditor: require('../ui/text-editor.vue')
    },
    methods: {
      addColumn()
      {
      let data = this.data;
        data.columns.push({
          title: 'Заголовок',
          body: 'Контент'
        });
        this.update(data);
      },
      setTitle(column, value)
      {
        let data = this.data;
        data.columns[column].title =  value;
        this.update(data);
      },
      setBody(column, value)
      {
        let data = this.data;
        data.columns[column].body =  value;
        this.update(data);
      },
      update(data) {
        this.$store.commit('constructor.updateSlide', {
          id: this.id,
          data
        });
      }
    }
  }
</script>

Both store actions update vuex state and this is seen in vuex inspector.
When I commit constructor.addSlide that pushes a new slide in the array — it updates both slides-previews and presentation-editor, however commiting constructor.updateSlide, that changes the data of some slide does not affect all instances:

_

The same in opposite way: if I change something in previews, state changes but editor is not affected.

Also even after constructor.addSlide previously changed slides don't update, only new one is added in both components

@ktsn

This comment has been minimized.

Show comment
Hide comment
@ktsn

ktsn Oct 23, 2016

Member

Hello, thanks for filing this issue.
Could you provide small live reproduction or project repository to debug this?

Member

ktsn commented Oct 23, 2016

Hello, thanks for filing this issue.
Could you provide small live reproduction or project repository to debug this?

@terion-name

This comment has been minimized.

Show comment
Hide comment
@terion-name

terion-name Oct 23, 2016

@ktsn I don't think that it is possible to fit all this in some fiddle.
I've temporary opened the repo (it's early-dev stage): http://gitlab.terion.name/kzmr/plan-n-go/tree/master
It can be cloned and run by:

$ yarn
$ gulp
$ gulp serve
// go to http://localhost:3000/#/constructor

A slide can be added with this button:
banners_and_alerts_ _ _

terion-name commented Oct 23, 2016

@ktsn I don't think that it is possible to fit all this in some fiddle.
I've temporary opened the repo (it's early-dev stage): http://gitlab.terion.name/kzmr/plan-n-go/tree/master
It can be cloned and run by:

$ yarn
$ gulp
$ gulp serve
// go to http://localhost:3000/#/constructor

A slide can be added with this button:
banners_and_alerts_ _ _

@ktsn

This comment has been minimized.

Show comment
Hide comment
@ktsn

ktsn Oct 24, 2016

Member

Thanks for providing your repository.
Took a quick look, there are a few problem in your code.

The main reason of not updating the view is you do not refer the store data in slide-idea. There is default data and it is used for rendering.
In addition, the store data is not passed to slide-idea. slide-preview passes slide.data on this line but the store data does not seem to have data property.

Member

ktsn commented Oct 24, 2016

Thanks for providing your repository.
Took a quick look, there are a few problem in your code.

The main reason of not updating the view is you do not refer the store data in slide-idea. There is default data and it is used for rendering.
In addition, the store data is not passed to slide-idea. slide-preview passes slide.data on this line but the store data does not seem to have data property.

@ktsn ktsn closed this Oct 24, 2016

@terion-name

This comment has been minimized.

Show comment
Hide comment
@terion-name

terion-name Oct 24, 2016

@ktsn thank you. Hm. I get the idea. As I understand, using data property for component is a bad idea in general due to possible conflicts with default component data?

terion-name commented Oct 24, 2016

@ktsn thank you. Hm. I get the idea. As I understand, using data property for component is a bad idea in general due to possible conflicts with default component data?

@ktsn

This comment has been minimized.

Show comment
Hide comment
@ktsn

ktsn Oct 24, 2016

Member

Yes, it's better to use props and keep components state-less as possible. Then, the application state management will be more simple and maintainable.

But sometimes we should use data to manage component specific state (e.g. a very simple form that would not affect other components). It's a trade-off between maintainability and code complexity etc.

Member

ktsn commented Oct 24, 2016

Yes, it's better to use props and keep components state-less as possible. Then, the application state management will be more simple and maintainable.

But sometimes we should use data to manage component specific state (e.g. a very simple form that would not affect other components). It's a trade-off between maintainability and code complexity etc.

@terion-name

This comment has been minimized.

Show comment
Hide comment
@terion-name

terion-name Oct 24, 2016

@ktsn thank you once more, I've changed property name to content, removed data() and now it seems to sync properly, but now there is another issue: when I type something in text boxes I get:

vue.common.js?4a36:352 [Vue warn]: Error in watcher "state" 
(found in root instance)warn @ vue.common.js?4a36:352run @ vue.common.js?4a36:170update @ vue.common.js?4a36:163notify @ vue.common.js?4a36:118reactiveSetter @ vue.common.js?4a36:224setTitle @ slide-idea.vue?6bb0:89boundFn @ vue.common.js?4a36:31change @ slide-idea.vue?2f32:14(anonymous function) @ vue.common.js?4a36:243Vue.$emit @ vue.common.js?4a36:343(anonymous function) @ text-editor.vue?2e06:36
vue.common.js?4a36:170 Uncaught Error: [vuex] Do not mutate vuex store state outside mutation handlers.

But the mutation is handled only by store handler:

mutations: {
        ['constructor.addSlide'](state, type) {
            const slides = Array.from(state.slides); // make a copy to get new reference
            slides.push({
                id: Math.round(Math.random() * 10000),
                type,
            });
            Vue.set(state, 'slides', slides);
        },
        ['constructor.updateSlide'](state, slide) {
            const slides = Array.from(state.slides); // make a copy to get new reference
            const targetSlide = slides.find(s => s.id === slide.id);
            if (targetSlide) targetSlide.data = slide.data;
            Vue.set(state, 'slides', slides);
        }
    }

Vue.set(state, 'slides', slides); and state.slides = slides; both generate errors.

If I remove them entirely vuex state changes (because there are objects that are referenced), but it does not trigger changes obviously.

And one thing to mention, that this error comes at second commit. First commit runs without errors.

terion-name commented Oct 24, 2016

@ktsn thank you once more, I've changed property name to content, removed data() and now it seems to sync properly, but now there is another issue: when I type something in text boxes I get:

vue.common.js?4a36:352 [Vue warn]: Error in watcher "state" 
(found in root instance)warn @ vue.common.js?4a36:352run @ vue.common.js?4a36:170update @ vue.common.js?4a36:163notify @ vue.common.js?4a36:118reactiveSetter @ vue.common.js?4a36:224setTitle @ slide-idea.vue?6bb0:89boundFn @ vue.common.js?4a36:31change @ slide-idea.vue?2f32:14(anonymous function) @ vue.common.js?4a36:243Vue.$emit @ vue.common.js?4a36:343(anonymous function) @ text-editor.vue?2e06:36
vue.common.js?4a36:170 Uncaught Error: [vuex] Do not mutate vuex store state outside mutation handlers.

But the mutation is handled only by store handler:

mutations: {
        ['constructor.addSlide'](state, type) {
            const slides = Array.from(state.slides); // make a copy to get new reference
            slides.push({
                id: Math.round(Math.random() * 10000),
                type,
            });
            Vue.set(state, 'slides', slides);
        },
        ['constructor.updateSlide'](state, slide) {
            const slides = Array.from(state.slides); // make a copy to get new reference
            const targetSlide = slides.find(s => s.id === slide.id);
            if (targetSlide) targetSlide.data = slide.data;
            Vue.set(state, 'slides', slides);
        }
    }

Vue.set(state, 'slides', slides); and state.slides = slides; both generate errors.

If I remove them entirely vuex state changes (because there are objects that are referenced), but it does not trigger changes obviously.

And one thing to mention, that this error comes at second commit. First commit runs without errors.

@terion-name

This comment has been minimized.

Show comment
Hide comment
@terion-name

terion-name Oct 24, 2016

@ktsn even calling splice() will trigger this error :(

['constructor.updateSlide'](state, slide) {
            const slides = Array.from(state.slides);
            const targetSlide = slides.find(s => s.id === slide.id);
            if (targetSlide) targetSlide.data = slide.data;
            state.slides.splice(); // try to trigger update
        }

terion-name commented Oct 24, 2016

@ktsn even calling splice() will trigger this error :(

['constructor.updateSlide'](state, slide) {
            const slides = Array.from(state.slides);
            const targetSlide = slides.find(s => s.id === slide.id);
            if (targetSlide) targetSlide.data = slide.data;
            state.slides.splice(); // try to trigger update
        }
@terion-name

This comment has been minimized.

Show comment
Hide comment
@terion-name

terion-name Oct 24, 2016

BTW disabling strict mode from store removed this errors

terion-name commented Oct 24, 2016

BTW disabling strict mode from store removed this errors

@ktsn

This comment has been minimized.

Show comment
Hide comment
@ktsn

ktsn Oct 24, 2016

Member

Looks like you are mutating store state in slide-idea.
http://gitlab.terion.name/kzmr/plan-n-go/blob/master/resources/assets/js/app/components/slides/slide-idea.vue#L89

Object.assign won't update nested object reference. So, data.columns is same reference as store state.

Member

ktsn commented Oct 24, 2016

Looks like you are mutating store state in slide-idea.
http://gitlab.terion.name/kzmr/plan-n-go/blob/master/resources/assets/js/app/components/slides/slide-idea.vue#L89

Object.assign won't update nested object reference. So, data.columns is same reference as store state.

@terion-name

This comment has been minimized.

Show comment
Hide comment
@terion-name

terion-name Oct 24, 2016

@ktsn ah. indeed. added deep-copy lib, seems to work fine now. Thank you so much.

Maybe some tooling for this should be added to core? For vuex-way it seems to be a very common case.

terion-name commented Oct 24, 2016

@ktsn ah. indeed. added deep-copy lib, seems to work fine now. Thank you so much.

Maybe some tooling for this should be added to core? For vuex-way it seems to be a very common case.

@ktsn

This comment has been minimized.

Show comment
Hide comment
@ktsn

ktsn Oct 25, 2016

Member

Well, I think we won't add deep-copy into the core because it would be bad idea to deep copy store state in a component.
You should divide the component and mutation to smaller parts and avoid passing entire module state as the mutation payload.

Member

ktsn commented Oct 25, 2016

Well, I think we won't add deep-copy into the core because it would be bad idea to deep copy store state in a component.
You should divide the component and mutation to smaller parts and avoid passing entire module state as the mutation payload.

@Stegosource

This comment has been minimized.

Show comment
Hide comment
@Stegosource

Stegosource Apr 26, 2017

@ktsn can you give an example of how you would implement your last comment? I also have a state with an array of objects that I need to update. Running into the same issue and not finding any solution to work well

Stegosource commented Apr 26, 2017

@ktsn can you give an example of how you would implement your last comment? I also have a state with an array of objects that I need to update. Running into the same issue and not finding any solution to work well

@connor11528

This comment has been minimized.

Show comment
Hide comment
@connor11528

connor11528 Sep 13, 2017

This seems fundamental to Vuex. I am trying to update state from a mutation but not having reactivity for updating the state

connor11528 commented Sep 13, 2017

This seems fundamental to Vuex. I am trying to update state from a mutation but not having reactivity for updating the state

@kirillgroshkov

This comment has been minimized.

Show comment
Hide comment
@kirillgroshkov

kirillgroshkov Feb 3, 2018

Ran into the same issue.
Solved by replacing the object one level higher than array in the store:

store.highLevelObject = {
  ...store.highLevelObject,
  key: newArray,
}

I don't know if it's relevant. But what I tried is to go with different ways to update store inside mutation, and this way finally worked. Not obvious at all.

kirillgroshkov commented Feb 3, 2018

Ran into the same issue.
Solved by replacing the object one level higher than array in the store:

store.highLevelObject = {
  ...store.highLevelObject,
  key: newArray,
}

I don't know if it's relevant. But what I tried is to go with different ways to update store inside mutation, and this way finally worked. Not obvious at all.

@redcpp

This comment has been minimized.

Show comment
Hide comment
@redcpp

redcpp Apr 19, 2018

Thanks to @kirillgroshkov I solved my problem!

In my case, I was changing an object inside an array. Say:

state.arr[id].status = !state.arr[id].status

What I had to do was force the array to change so that it triggers a re-render:

state.arr = state.arr.map((e, i) => { if (i === id) { e.status = !e.status } return e })

redcpp commented Apr 19, 2018

Thanks to @kirillgroshkov I solved my problem!

In my case, I was changing an object inside an array. Say:

state.arr[id].status = !state.arr[id].status

What I had to do was force the array to change so that it triggers a re-render:

state.arr = state.arr.map((e, i) => { if (i === id) { e.status = !e.status } return e })

@gapsong

This comment has been minimized.

Show comment
Hide comment
@gapsong

gapsong commented Jul 17, 2018

Thank you @redcpp !

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