Merge branch 'fl-mr-refactor-performance-improvements' into 'master'

Improves MR refactor getters and state and adds specs

Closes #48937

See merge request gitlab-org/gitlab-ce!20429

8 files changed, 90 insertions, 55 deletions

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);
     });
   });
 });