From 9eef6cd1bf0b980c36d5065e30398949485b74bb Mon Sep 17 00:00:00 2001
From: Tim Zallmann <tzallmann@gitlab.com>
Date: Mon, 9 Jul 2018 11:54:15 +0000
Subject: Resolve "MR refactor: Performance costs of main element with `v-if`"

---
 .../diffs/components/diff_discussions.vue          |  4 +--
 .../javascripts/diffs/components/diff_file.vue     |  7 +++--
 .../diffs/components/inline_diff_comment_row.vue   | 15 +---------
 .../diffs/components/inline_diff_view.vue          | 20 +++++++++++--
 .../diffs/components/parallel_diff_comment_row.vue | 10 ++-----
 .../diffs/components/parallel_diff_view.vue        | 34 ++++++++++++++++++++--
 .../javascripts/diffs/components/diff_file_spec.js |  4 +--
 7 files changed, 61 insertions(+), 33 deletions(-)

diff --git a/app/assets/javascripts/diffs/components/diff_discussions.vue b/app/assets/javascripts/diffs/components/diff_discussions.vue
index 39d535036f6..20483161033 100644
--- a/app/assets/javascripts/diffs/components/diff_discussions.vue
+++ b/app/assets/javascripts/diffs/components/diff_discussions.vue
@@ -15,9 +15,7 @@ export default {
 </script>
 
 <template>
-  <div
-    v-if="discussions.length"
-  >
+  <div>
     <div
       v-for="discussion in discussions"
       :key="discussion.id"
diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue
index 108eefdac5f..b5a7392830e 100644
--- a/app/assets/javascripts/diffs/components/diff_file.vue
+++ b/app/assets/javascripts/diffs/components/diff_file.vue
@@ -47,6 +47,9 @@ export default {
         false,
       );
     },
+    showExpandMessage() {
+      return this.isCollapsed && !this.isLoadingCollapsedDiff && !this.file.tooLarge;
+    },
   },
   mounted() {
     document.addEventListener('scroll', this.handleScroll);
@@ -159,7 +162,7 @@ export default {
     </div>
 
     <diff-content
-      v-show="!isCollapsed"
+      v-if="!isCollapsed"
       :class="{ hidden: isCollapsed || file.tooLarge }"
       :diff-file="file"
     />
@@ -168,7 +171,7 @@ export default {
       class="diff-content loading"
     />
     <div
-      v-show="isCollapsed && !isLoadingCollapsedDiff && !file.tooLarge"
+      v-if="showExpandMessage"
       class="nothing-here-block diff-collapsed"
     >
       {{ __('This diff is collapsed.') }}
diff --git a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue
index 0e935f1d68e..1e8f2eecd76 100644
--- a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue
+++ b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue
@@ -31,22 +31,9 @@ export default {
       diffLineCommentForms: state => state.diffs.diffLineCommentForms,
     }),
     ...mapGetters(['discussionsByLineCode']),
-    isDiscussionExpanded() {
-      if (!this.discussions.length) {
-        return false;
-      }
-
-      return this.discussions.every(discussion => discussion.expanded);
-    },
-    hasCommentForm() {
-      return this.diffLineCommentForms[this.line.lineCode];
-    },
     discussions() {
       return this.discussionsByLineCode[this.line.lineCode] || [];
     },
-    shouldRender() {
-      return this.isDiscussionExpanded || this.hasCommentForm;
-    },
     className() {
       return this.discussions.length ? '' : 'js-temp-notes-holder';
     },
@@ -56,7 +43,6 @@ export default {
 
 <template>
   <tr
-    v-if="shouldRender"
     :class="className"
     class="notes_holder"
   >
@@ -67,6 +53,7 @@ export default {
     <td class="notes_content">
       <div class="content">
         <diff-discussions
+          v-if="discussions.length"
           :discussions="discussions"
         />
         <diff-line-note-form
diff --git a/app/assets/javascripts/diffs/components/inline_diff_view.vue b/app/assets/javascripts/diffs/components/inline_diff_view.vue
index 83569346cf8..fcaa6537d03 100644
--- a/app/assets/javascripts/diffs/components/inline_diff_view.vue
+++ b/app/assets/javascripts/diffs/components/inline_diff_view.vue
@@ -1,5 +1,5 @@
 <script>
-import { mapGetters } from 'vuex';
+import { mapGetters, mapState } from 'vuex';
 import inlineDiffTableRow from './inline_diff_table_row.vue';
 import inlineDiffCommentRow from './inline_diff_comment_row.vue';
 import { trimFirstCharOfLineContent } from '../store/utils';
@@ -20,7 +20,10 @@ export default {
     },
   },
   computed: {
-    ...mapGetters(['commitId']),
+    ...mapGetters(['commitId', 'discussionsByLineCode']),
+    ...mapState({
+      diffLineCommentForms: state => state.diffs.diffLineCommentForms,
+    }),
     normalizedDiffLines() {
       return this.diffLines.map(line => (line.richText ? trimFirstCharOfLineContent(line) : line));
     },
@@ -31,6 +34,18 @@ export default {
       return window.gon.user_color_scheme;
     },
   },
+  methods: {
+    shouldRenderCommentRow(line) {
+      if (this.diffLineCommentForms[line.lineCode]) return true;
+
+      const lineDiscussions = this.discussionsByLineCode[line.lineCode];
+      if (lineDiscussions === undefined) {
+        return false;
+      }
+
+      return lineDiscussions.every(discussion => discussion.expanded);
+    },
+  },
 };
 </script>
 
@@ -50,6 +65,7 @@ export default {
           :key="line.lineCode"
         />
         <inline-diff-comment-row
+          v-if="shouldRenderCommentRow(line)"
           :diff-file="diffFile"
           :diff-lines="normalizedDiffLines"
           :line="line"
diff --git a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue
index 5f33ec7a3c2..1e20792b647 100644
--- a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue
+++ b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue
@@ -55,13 +55,6 @@ export default {
     hasAnyExpandedDiscussion() {
       return this.hasExpandedDiscussionOnLeft || this.hasExpandedDiscussionOnRight;
     },
-    shouldRenderDiscussionsRow() {
-      const hasDiscussion = this.hasDiscussion && this.hasAnyExpandedDiscussion;
-      const hasCommentFormOnLeft = this.diffLineCommentForms[this.leftLineCode];
-      const hasCommentFormOnRight = this.diffLineCommentForms[this.rightLineCode];
-
-      return hasDiscussion || hasCommentFormOnLeft || hasCommentFormOnRight;
-    },
     shouldRenderDiscussionsOnLeft() {
       return this.discussionsByLineCode[this.leftLineCode] && this.hasExpandedDiscussionOnLeft;
     },
@@ -81,7 +74,6 @@ export default {
 
 <template>
   <tr
-    v-if="shouldRenderDiscussionsRow"
     :class="className"
     class="notes_holder"
   >
@@ -92,6 +84,7 @@ export default {
         class="content"
       >
         <diff-discussions
+          v-if="discussionsByLineCode[leftLineCode].length"
           :discussions="discussionsByLineCode[leftLineCode]"
         />
       </div>
@@ -112,6 +105,7 @@ export default {
         class="content"
       >
         <diff-discussions
+          v-if="discussionsByLineCode[rightLineCode].length"
           :discussions="discussionsByLineCode[rightLineCode]"
         />
       </div>
diff --git a/app/assets/javascripts/diffs/components/parallel_diff_view.vue b/app/assets/javascripts/diffs/components/parallel_diff_view.vue
index 89148eb5e18..569fc8b8e47 100644
--- a/app/assets/javascripts/diffs/components/parallel_diff_view.vue
+++ b/app/assets/javascripts/diffs/components/parallel_diff_view.vue
@@ -1,5 +1,5 @@
 <script>
-import { mapGetters } from 'vuex';
+import { mapState, mapGetters } from 'vuex';
 import parallelDiffTableRow from './parallel_diff_table_row.vue';
 import parallelDiffCommentRow from './parallel_diff_comment_row.vue';
 import { EMPTY_CELL_TYPE } from '../constants';
@@ -21,7 +21,10 @@ export default {
     },
   },
   computed: {
-    ...mapGetters(['commitId']),
+    ...mapGetters(['commitId', 'discussionsByLineCode']),
+    ...mapState({
+      diffLineCommentForms: state => state.diffs.diffLineCommentForms,
+    }),
     parallelDiffLines() {
       return this.diffLines.map(line => {
         const parallelLine = Object.assign({}, line);
@@ -48,6 +51,32 @@ export default {
       return window.gon.user_color_scheme;
     },
   },
+  methods: {
+    shouldRenderCommentRow(line) {
+      const leftLineCode = line.left.lineCode;
+      const rightLineCode = line.right.lineCode;
+      const discussions = this.discussionsByLineCode;
+      const leftDiscussions = discussions[leftLineCode];
+      const rightDiscussions = discussions[rightLineCode];
+      const hasDiscussion = leftDiscussions || rightDiscussions;
+
+      const hasExpandedDiscussionOnLeft = leftDiscussions
+        ? leftDiscussions.every(discussion => discussion.expanded)
+        : false;
+      const hasExpandedDiscussionOnRight = rightDiscussions
+        ? rightDiscussions.every(discussion => discussion.expanded)
+        : false;
+
+      if (hasDiscussion && (hasExpandedDiscussionOnLeft || hasExpandedDiscussionOnRight)) {
+        return true;
+      }
+
+      const hasCommentFormOnLeft = this.diffLineCommentForms[leftLineCode];
+      const hasCommentFormOnRight = this.diffLineCommentForms[rightLineCode];
+
+      return hasCommentFormOnLeft || hasCommentFormOnRight;
+    },
+  },
 };
 </script>
 
@@ -69,6 +98,7 @@ export default {
             :key="index"
           />
           <parallel-diff-comment-row
+            v-if="shouldRenderCommentRow(line)"
             :key="line.left.lineCode || line.right.lineCode"
             :line="line"
             :diff-file="diffFile"
diff --git a/spec/javascripts/diffs/components/diff_file_spec.js b/spec/javascripts/diffs/components/diff_file_spec.js
index 1c1edfac68c..9b994543e19 100644
--- a/spec/javascripts/diffs/components/diff_file_spec.js
+++ b/spec/javascripts/diffs/components/diff_file_spec.js
@@ -31,12 +31,12 @@ describe('DiffFile', () => {
 
     describe('collapsed', () => {
       it('should not have file content', done => {
-        expect(vm.$el.querySelectorAll('.diff-content.hidden').length).toEqual(0);
+        expect(vm.$el.querySelectorAll('.diff-content').length).toEqual(1);
         expect(vm.file.collapsed).toEqual(false);
         vm.file.collapsed = true;
 
         vm.$nextTick(() => {
-          expect(vm.$el.querySelectorAll('.diff-content.hidden').length).toEqual(1);
+          expect(vm.$el.querySelectorAll('.diff-content').length).toEqual(0);
 
           done();
         });
-- 
cgit v1.2.1