From adb7f45affd71020b8b5b7f8470671c8947b6869 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 6 Jul 2018 13:13:33 +0100 Subject: [PATCH] Exports getters individually. Exports state to allow tests Adds specs for the getters that didn't have any. --- .../diffs/components/inline_diff_view.vue | 5 +- .../diffs/components/parallel_diff_view.vue | 5 +- app/assets/javascripts/diffs/store/getters.js | 24 ++++----- .../diffs/store/modules/diff_state.js | 18 +++++++ .../javascripts/diffs/store/modules/index.js | 21 ++------ .../javascripts/diffs/store/mutations.js | 15 ++---- ...l-mr-refactor-performance-improvements.yml | 5 ++ spec/javascripts/diffs/store/getters_spec.js | 52 +++++++++++++++++-- 8 files changed, 90 insertions(+), 55 deletions(-) create mode 100644 app/assets/javascripts/diffs/store/modules/diff_state.js create mode 100644 changelogs/unreleased/fl-mr-refactor-performance-improvements.yml diff --git a/app/assets/javascripts/diffs/components/inline_diff_view.vue b/app/assets/javascripts/diffs/components/inline_diff_view.vue index b884230fb63..83569346cf8 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_view.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_view.vue @@ -20,16 +20,13 @@ export default { }, }, computed: { - ...mapGetters(['commit']), + ...mapGetters(['commitId']), normalizedDiffLines() { return this.diffLines.map(line => (line.richText ? trimFirstCharOfLineContent(line) : line)); }, diffLinesLength() { return this.normalizedDiffLines.length; }, - commitId() { - return this.commit && this.commit.id; - }, userColorScheme() { return window.gon.user_color_scheme; }, diff --git a/app/assets/javascripts/diffs/components/parallel_diff_view.vue b/app/assets/javascripts/diffs/components/parallel_diff_view.vue index 52561e197e6..89148eb5e18 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_view.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_view.vue @@ -21,7 +21,7 @@ export default { }, }, computed: { - ...mapGetters(['commit']), + ...mapGetters(['commitId']), parallelDiffLines() { return this.diffLines.map(line => { const parallelLine = Object.assign({}, line); @@ -44,9 +44,6 @@ export default { diffLinesLength() { return this.parallelDiffLines.length; }, - commitId() { - return this.commit && this.commit.id; - }, userColorScheme() { return window.gon.user_color_scheme; }, diff --git a/app/assets/javascripts/diffs/store/getters.js b/app/assets/javascripts/diffs/store/getters.js index 66d0f47d102..f3c2d7427e7 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -1,16 +1,12 @@ import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '../constants'; -export default { - isParallelView(state) { - return state.diffViewType === PARALLEL_DIFF_VIEW_TYPE; - }, - isInlineView(state) { - return state.diffViewType === INLINE_DIFF_VIEW_TYPE; - }, - areAllFilesCollapsed(state) { - return state.diffFiles.every(file => file.collapsed); - }, - commit(state) { - return state.commit; - }, -}; +export const isParallelView = state => state.diffViewType === PARALLEL_DIFF_VIEW_TYPE; + +export const isInlineView = state => state.diffViewType === INLINE_DIFF_VIEW_TYPE; + +export const areAllFilesCollapsed = state => state.diffFiles.every(file => file.collapsed); + +export const commitId = state => (state.commit && state.commit.id ? state.commit.id : null); + +// prevent babel-plugin-rewire from generating an invalid default during karma tests +export default () => {}; diff --git a/app/assets/javascripts/diffs/store/modules/diff_state.js b/app/assets/javascripts/diffs/store/modules/diff_state.js new file mode 100644 index 00000000000..39d90a64aab --- /dev/null +++ b/app/assets/javascripts/diffs/store/modules/diff_state.js @@ -0,0 +1,18 @@ +import Cookies from 'js-cookie'; +import { getParameterValues } from '~/lib/utils/url_utility'; +import { INLINE_DIFF_VIEW_TYPE, DIFF_VIEW_COOKIE_NAME } from '../../constants'; + +const viewTypeFromQueryString = getParameterValues('view')[0]; +const viewTypeFromCookie = Cookies.get(DIFF_VIEW_COOKIE_NAME); +const defaultViewType = INLINE_DIFF_VIEW_TYPE; + +export default () => ({ + isLoading: true, + endpoint: '', + basePath: '', + commit: null, + diffFiles: [], + mergeRequestDiffs: [], + diffLineCommentForms: {}, + diffViewType: viewTypeFromQueryString || viewTypeFromCookie || defaultViewType, +}); diff --git a/app/assets/javascripts/diffs/store/modules/index.js b/app/assets/javascripts/diffs/store/modules/index.js index 94caa131506..c745320d532 100644 --- a/app/assets/javascripts/diffs/store/modules/index.js +++ b/app/assets/javascripts/diffs/store/modules/index.js @@ -1,25 +1,10 @@ -import Cookies from 'js-cookie'; -import { getParameterValues } from '~/lib/utils/url_utility'; import actions from '../actions'; -import getters from '../getters'; +import * as getters from '../getters'; import mutations from '../mutations'; -import { INLINE_DIFF_VIEW_TYPE, DIFF_VIEW_COOKIE_NAME } from '../../constants'; - -const viewTypeFromQueryString = getParameterValues('view')[0]; -const viewTypeFromCookie = Cookies.get(DIFF_VIEW_COOKIE_NAME); -const defaultViewType = INLINE_DIFF_VIEW_TYPE; +import createState from './diff_state'; export default { - state: { - isLoading: true, - endpoint: '', - basePath: '', - commit: null, - diffFiles: [], - mergeRequestDiffs: [], - diffLineCommentForms: {}, - diffViewType: viewTypeFromQueryString || viewTypeFromCookie || defaultViewType, - }, + state: createState(), getters, actions, mutations, diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 8aa8a114c6f..a98b2be89a3 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -66,15 +66,10 @@ export default { }, [types.EXPAND_ALL_FILES](state) { - const diffFiles = []; - - state.diffFiles.forEach(file => { - diffFiles.push({ - ...file, - collapsed: false, - }); - }); - - Object.assign(state, { diffFiles }); + // eslint-disable-next-line no-param-reassign + state.diffFiles = state.diffFiles.map(file => ({ + ...file, + collapsed: false, + })); }, }; diff --git a/changelogs/unreleased/fl-mr-refactor-performance-improvements.yml b/changelogs/unreleased/fl-mr-refactor-performance-improvements.yml new file mode 100644 index 00000000000..649d1b5da67 --- /dev/null +++ b/changelogs/unreleased/fl-mr-refactor-performance-improvements.yml @@ -0,0 +1,5 @@ +--- +title: Structure getters for diff Store properly and adds specs +merge_request: +author: +type: fixed diff --git a/spec/javascripts/diffs/store/getters_spec.js b/spec/javascripts/diffs/store/getters_spec.js index 7945ddea911..7a94f18778b 100644 --- a/spec/javascripts/diffs/store/getters_spec.js +++ b/spec/javascripts/diffs/store/getters_spec.js @@ -1,24 +1,66 @@ -import getters from '~/diffs/store/getters'; +import * as getters from '~/diffs/store/getters'; +import state from '~/diffs/store/modules/diff_state'; import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '~/diffs/constants'; describe('DiffsStoreGetters', () => { + let localState; + + beforeEach(() => { + localState = state(); + }); + describe('isParallelView', () => { it('should return true if view set to parallel view', () => { - expect(getters.isParallelView({ diffViewType: PARALLEL_DIFF_VIEW_TYPE })).toBeTruthy(); + localState.diffViewType = PARALLEL_DIFF_VIEW_TYPE; + + expect(getters.isParallelView(localState)).toEqual(true); }); it('should return false if view not to parallel view', () => { - expect(getters.isParallelView({ diffViewType: 'foo' })).toBeFalsy(); + localState.diffViewType = INLINE_DIFF_VIEW_TYPE; + + expect(getters.isParallelView(localState)).toEqual(false); }); }); describe('isInlineView', () => { it('should return true if view set to inline view', () => { - expect(getters.isInlineView({ diffViewType: INLINE_DIFF_VIEW_TYPE })).toBeTruthy(); + localState.diffViewType = INLINE_DIFF_VIEW_TYPE; + + expect(getters.isInlineView(localState)).toEqual(true); }); it('should return false if view not to inline view', () => { - expect(getters.isInlineView({ diffViewType: PARALLEL_DIFF_VIEW_TYPE })).toBeFalsy(); + localState.diffViewType = PARALLEL_DIFF_VIEW_TYPE; + + expect(getters.isInlineView(localState)).toEqual(false); + }); + }); + + describe('areAllFilesCollapsed', () => { + it('returns true when all files are collapsed', () => { + localState.diffFiles = [{ collapsed: true }, { collapsed: true }]; + expect(getters.areAllFilesCollapsed(localState)).toEqual(true); + }); + + it('returns false when at least one file is not collapsed', () => { + localState.diffFiles = [{ collapsed: false }, { collapsed: true }]; + expect(getters.areAllFilesCollapsed(localState)).toEqual(false); + }); + }); + + describe('commitId', () => { + it('returns commit id when is set', () => { + const commitID = '800f7a91'; + localState.commit = { + id: commitID, + }; + + expect(getters.commitId(localState)).toEqual(commitID); + }); + + it('returns null when no commit is set', () => { + expect(getters.commitId(localState)).toEqual(null); }); }); });