Use vuex instead EventBus #809
Conversation
|
Some tests are failing, can you address that? |
| @@ -137,8 +136,11 @@ export default { | |||
| mounted() { | |||
| this.reloadData(); | |||
|
|
|||
| EventBus.$on('reloadData', () => { | |||
| this.reloadData(); | |||
| this.$watch(() => this.reloading, (reload) => { | |||
There was a problem hiding this comment.
looks like overhead to me, whats the benefit of this change?
There was a problem hiding this comment.
If I understood correctly the removal of the event bus.
There was a problem hiding this comment.
the benefit of removing the event bus is removing the event bus, whats the benefit of this change actually?
| @@ -53,7 +52,7 @@ export default { | |||
| (data) => client.signMessage(data), | |||
| this.parentId, | |||
| ); | |||
| EventBus.$emit('reloadData'); | |||
| this.$store.commit('reloading', true); | |||
There was a problem hiding this comment.
with committing to the store to me its not implicit whats happening, emitting some event is way more clear
There was a problem hiding this comment.
What is your proposal? Should we leave the old implementation?
There was a problem hiding this comment.
my proposal is to remove the event bus, but not by emulating an event bus with vuex, but use vuex as intended or imagined (cc @davidyuk )
| @@ -5,6 +5,7 @@ import mutations from './mutations'; | |||
| import backend from './modules/backend'; | |||
| import persistState from './plugins/persistState'; | |||
| import modals from './plugins/modals'; | |||
| /* eslint import/no-cycle: [2, { maxDepth: 1 }] */ | |||
There was a problem hiding this comment.
we should avoid cyclic dependencies
There was a problem hiding this comment.
I agree. To avoid this, most of the current implementation needs to be changed.
There was a problem hiding this comment.
yes, that would be necessary
|
As i undestood, the main idea of dropping EvenBus from Denis, was to drop this workaround behaviour of emitting reset from each component. |
|
As @CedrikNikita @davidyuk say, I think we should keep a event emit if we need event emit, but have the store update the state |
close #660