Commit 49828db5 authored by Filipa Lacerda's avatar Filipa Lacerda Committed by Phil Hughes

Improves performance on MR refactor and adds specs

parent 8b0e2628
...@@ -24,19 +24,21 @@ export default { ...@@ -24,19 +24,21 @@ export default {
...mapGetters(['commit']), ...mapGetters(['commit']),
parallelDiffLines() { parallelDiffLines() {
return this.diffLines.map(line => { return this.diffLines.map(line => {
const parallelLine = Object.assign({}, line);
if (line.left) { if (line.left) {
Object.assign(line, { left: trimFirstCharOfLineContent(line.left) }); parallelLine.left = trimFirstCharOfLineContent(line.left);
} else { } else {
Object.assign(line, { left: { type: EMPTY_CELL_TYPE } }); parallelLine.left = { type: EMPTY_CELL_TYPE };
} }
if (line.right) { if (line.right) {
Object.assign(line, { right: trimFirstCharOfLineContent(line.right) }); parallelLine.right = trimFirstCharOfLineContent(line.right);
} else { } else {
Object.assign(line, { right: { type: EMPTY_CELL_TYPE } }); parallelLine.right = { type: EMPTY_CELL_TYPE };
} }
return line; return parallelLine;
}); });
}, },
diffLinesLength() { diffLinesLength() {
......
...@@ -155,18 +155,21 @@ export function addContextLines(options) { ...@@ -155,18 +155,21 @@ export function addContextLines(options) {
} }
} }
export function trimFirstCharOfLineContent(line) { /**
if (!line.richText) { * Trims the first char of the `richText` property when it's either a space or a diff symbol.
return line; * @param {Object} line
} * @returns {Object}
*/
const firstChar = line.richText.charAt(0); export function trimFirstCharOfLineContent(line = {}) {
const parsedLine = Object.assign({}, line);
if (firstChar === ' ' || firstChar === '+' || firstChar === '-') {
Object.assign(line, { if (line.richText) {
richText: line.richText.substring(1), const firstChar = parsedLine.richText.charAt(0);
});
if (firstChar === ' ' || firstChar === '+' || firstChar === '-') {
parsedLine.richText = line.richText.substring(1);
}
} }
return line; return parsedLine;
} }
<script> <script>
import { mapState, mapActions } from 'vuex'; import { mapState, mapActions } from 'vuex';
import imageDiffHelper from '~/image_diff/helpers/index'; import imageDiffHelper from '~/image_diff/helpers/index';
import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils'; import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
import DiffFileHeader from '~/diffs/components/diff_file_header.vue'; import DiffFileHeader from '~/diffs/components/diff_file_header.vue';
import SkeletonLoadingContainer from '~/vue_shared/components/skeleton_loading_container.vue'; import SkeletonLoadingContainer from '~/vue_shared/components/skeleton_loading_container.vue';
import { trimFirstCharOfLineContent } from '~/diffs/store/utils'; import { trimFirstCharOfLineContent } from '~/diffs/store/utils';
export default { export default {
components: { components: {
DiffFileHeader, DiffFileHeader,
SkeletonLoadingContainer, SkeletonLoadingContainer,
},
props: {
discussion: {
type: Object,
required: true,
}, },
}, props: {
data() { discussion: {
return { type: Object,
error: false, required: true,
}; },
},
computed: {
...mapState({
noteableData: state => state.notes.noteableData,
}),
hasTruncatedDiffLines() {
return this.discussion.truncatedDiffLines && this.discussion.truncatedDiffLines.length !== 0;
}, },
isDiscussionsExpanded() { data() {
return true; // TODO: @fatihacet - Fix this. return {
error: false,
};
}, },
isCollapsed() { computed: {
return this.diffFile.collapsed || false; ...mapState({
}, noteableData: state => state.notes.noteableData,
isImageDiff() { }),
return !this.diffFile.text; hasTruncatedDiffLines() {
}, return this.discussion.truncatedDiffLines &&
diffFileClass() { this.discussion.truncatedDiffLines.length !== 0;
const { text } = this.diffFile; },
return text ? 'text-file' : 'js-image-file'; isDiscussionsExpanded() {
}, return true; // TODO: @fatihacet - Fix this.
diffFile() { },
return convertObjectPropsToCamelCase(this.discussion.diffFile, { deep: true }); isCollapsed() {
}, return this.diffFile.collapsed || false;
imageDiffHtml() { },
return this.discussion.imageDiffHtml; isImageDiff() {
}, return !this.diffFile.text;
currentUser() { },
return this.noteableData.current_user; diffFileClass() {
}, const { text } = this.diffFile;
userColorScheme() { return text ? 'text-file' : 'js-image-file';
return window.gon.user_color_scheme; },
}, diffFile() {
normalizedDiffLines() { return convertObjectPropsToCamelCase(this.discussion.diffFile, { deep: true });
const lines = this.discussion.truncatedDiffLines || []; },
imageDiffHtml() {
return this.discussion.imageDiffHtml;
},
currentUser() {
return this.noteableData.current_user;
},
userColorScheme() {
return window.gon.user_color_scheme;
},
normalizedDiffLines() {
if (this.discussion.truncatedDiffLines) {
return this.discussion.truncatedDiffLines.map(line =>
trimFirstCharOfLineContent(convertObjectPropsToCamelCase(line)),
);
}
return lines.map(line => trimFirstCharOfLineContent(convertObjectPropsToCamelCase(line))); return [];
},
}, },
}, mounted() {
mounted() { if (this.isImageDiff) {
if (this.isImageDiff) { const canCreateNote = false;
const canCreateNote = false; const renderCommentBadge = true;
const renderCommentBadge = true; imageDiffHelper.initImageDiff(this.$refs.fileHolder, canCreateNote, renderCommentBadge);
imageDiffHelper.initImageDiff(this.$refs.fileHolder, canCreateNote, renderCommentBadge); } else if (!this.hasTruncatedDiffLines) {
} else if (!this.hasTruncatedDiffLines) { this.fetchDiff();
this.fetchDiff(); }
}
},
methods: {
...mapActions(['fetchDiscussionDiffLines']),
rowTag(html) {
return html.outerHTML ? 'tr' : 'template';
}, },
fetchDiff() { methods: {
this.error = false; ...mapActions(['fetchDiscussionDiffLines']),
this.fetchDiscussionDiffLines(this.discussion) rowTag(html) {
.then(this.highlight) return html.outerHTML ? 'tr' : 'template';
.catch(() => { },
this.error = true; fetchDiff() {
}); this.error = false;
this.fetchDiscussionDiffLines(this.discussion)
.then(this.highlight)
.catch(() => {
this.error = true;
});
},
}, },
}, };
};
</script> </script>
<template> <template>
......
---
title: Improves performance of mr code, by fixing the state being mutated outside
of the store in the util function trimFirstCharOfLineContent and in map operations.
Avoids map operation in an empty array. Adds specs to the trimFirstCharOfLineContent
function
merge_request: 20380
author: filipa
type: performance
...@@ -176,4 +176,35 @@ describe('DiffsStoreUtils', () => { ...@@ -176,4 +176,35 @@ describe('DiffsStoreUtils', () => {
expect(linesWithReferences[1].metaData.newPos).toEqual(3); expect(linesWithReferences[1].metaData.newPos).toEqual(3);
}); });
}); });
describe('trimFirstCharOfLineContent', () => {
it('trims the line when it starts with a space', () => {
expect(utils.trimFirstCharOfLineContent({ richText: ' diff' })).toEqual({ richText: 'diff' });
});
it('trims the line when it starts with a +', () => {
expect(utils.trimFirstCharOfLineContent({ richText: '+diff' })).toEqual({ richText: 'diff' });
});
it('trims the line when it starts with a -', () => {
expect(utils.trimFirstCharOfLineContent({ richText: '-diff' })).toEqual({ richText: 'diff' });
});
it('does not trims the line when it starts with a letter', () => {
expect(utils.trimFirstCharOfLineContent({ richText: 'diff' })).toEqual({ richText: 'diff' });
});
it('does not modify the provided object', () => {
const lineObj = {
richText: ' diff',
};
utils.trimFirstCharOfLineContent(lineObj);
expect(lineObj).toEqual({ richText: ' diff' });
});
it('handles a undefined or null parameter', () => {
expect(utils.trimFirstCharOfLineContent()).toEqual({});
});
});
}); });
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment