Change: revise Vuex actions guidelines
Old Pattern
Our Vuex guidelines state that any change to our state should be described as an action. Let's take a look at the example there:
export const requestUsers = ({ commit }) => commit(types.REQUEST_USERS);
export const receiveUsersSuccess = ({ commit }, data) => commit(types.RECEIVE_USERS_SUCCESS, data);
export const receiveUsersError = ({ commit }, error) => commit(types.RECEIVE_USERS_ERROR, error);
export const fetchUsers = ({ state, dispatch }) => {
dispatch('requestUsers');
axios.get(state.endpoint)
.then(({ data }) => dispatch('receiveUsersSuccess', data))
.catch((error) => {
dispatch('receiveUsersError', error)
createFlash('There was an error')
});
}
New Pattern
Changes to the state are performed by mutations. If we only need to perform the synchronous change to the state and we want to call this change from any action, we don't need to create additional actions that only call respective mutations.
export const fetchUsers = ({ state, dispatch, commit }) => {
commit(types.REQUEST_USERS);
axios.get(state.endpoint)
.then(({ data }) => commit(types.RECEIVE_USERS_SUCCESS, data))
.catch((error) => {
commit(types.RECEIVE_USERS_ERROR, error)
createFlash('There was an error')
});
}
This pattern doesn't apply to components: even if you need to perform the synchronous change to the store and you want to trigger it from the component, you need to use an action.
Also, I don't think we should invest in refactoring all the code to the new pattern: let's follow it when adding a new code / applying some changes to the old ones. Also, we're already using a new pattern a lot across the codebase
Advantages of switching patterns
The code is way less verbose and bloated, it's easier to follow and it matches official Vuex documentation so it will create less confusion
Disadvantages of switching patterns
I don't think I can find ane as we're already breaking this pattern in our codebase
What is the impact on our existing codebase?
None. We will try to follow the 'new' pattern on new MRs and won't consider breaking 'old' pattern as an error when reviewing MRs.
Reference implementation
Cases from our codebase:
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/javascripts/diffs/store/actions.js#L90
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/assets/javascripts/pages/groups/saml_providers/saml_members/store/actions.js#L16
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/assets/javascripts/issues_analytics/stores/modules/issue_analytics/actions.js#L20
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/assets/javascripts/ide/stores/modules/terminal_sync/actions.js#L10
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js#L20
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/javascripts/registry/explorer/stores/actions.js#L39
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/javascripts/pipelines/stores/test_reports/actions.js#L15
- https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/javascripts/logs/stores/actions.js#L99
...and many more