From 92adcd4483c692f6a5c7fe2c257563eaaa17ce5a Mon Sep 17 00:00:00 2001 From: Jacques Date: Tue, 7 Feb 2023 14:30:48 +0100 Subject: [PATCH 1/3] Create highlight mixin Created a highlight mixin to interface with the highlight worker --- .../repository/mixins/highlight_mixin.js | 88 +++++++++++++++ .../repository/mixins/highlight_mixin_spec.js | 105 ++++++++++++++++++ 2 files changed, 193 insertions(+) create mode 100644 app/assets/javascripts/repository/mixins/highlight_mixin.js create mode 100644 spec/frontend/repository/mixins/highlight_mixin_spec.js diff --git a/app/assets/javascripts/repository/mixins/highlight_mixin.js b/app/assets/javascripts/repository/mixins/highlight_mixin.js new file mode 100644 index 00000000000000..fc1fef724da46d --- /dev/null +++ b/app/assets/javascripts/repository/mixins/highlight_mixin.js @@ -0,0 +1,88 @@ +import { + LEGACY_FALLBACKS, + EVENT_ACTION, + EVENT_LABEL_FALLBACK, + LINES_PER_CHUNK, +} from '~/vue_shared/components/source_viewer/constants'; +import { splitIntoChunks } from '~/vue_shared/components/source_viewer/workers/highlight_utils'; +import LineHighlighter from '~/blob/line_highlighter'; +import languageLoader from '~/content_editor/services/highlight_js_language_loader'; +import Tracking from '~/tracking'; +import { TEXT_FILE_TYPE } from '../constants'; + +/* + * This mixin is intended to be used as an interface between our highlight worker and Vue components + */ +export default { + mixins: [Tracking.mixin()], + inject: { + highlightWorker: { default: null }, + }, + data() { + return { + chunks: [], + }; + }, + methods: { + trackEvent(label, language) { + this.track(EVENT_ACTION, { label, property: language }); + }, + unsupportedLanguage(language) { + const supportedLanguages = Object.keys(languageLoader); + const unsupportedLanguage = !supportedLanguages.includes(language); + + return LEGACY_FALLBACKS.includes(language) || unsupportedLanguage; + }, + handleUnsupportedLanguage(language) { + this.trackEvent(EVENT_LABEL_FALLBACK, language); + this.onError(); + }, + initHighlightWorker({ rawTextBlob, language, simpleViewer }) { + if (simpleViewer?.fileType !== TEXT_FILE_TYPE) return; + + if (this.unsupportedLanguage(language)) { + this.handleUnsupportedLanguage(language); + return; + } + + // Render the first 70 lines (raw text) ASAP, this improves perceived performance and LCP. + const firstSeventyLines = rawTextBlob.split(/\r?\n/).slice(0, LINES_PER_CHUNK).join('\n'); + + this.chunks = splitIntoChunks(language, firstSeventyLines); + + this.highlightWorker.onmessage = this.handleWorkerMessage; + + // Instruct the worker to highlight the first 70 lines ASAP, this improves perceived performance. + this.instructWorker(firstSeventyLines, language); + + // Instruct the worker to start highlighting all lines in the background. + this.instructWorker(rawTextBlob, language); + }, + handleWorkerMessage({ data }) { + this.chunks = data; + this.highlightHash(); // highlight the line is a line number hash is present in the URL + }, + instructWorker(content, language) { + this.highlightWorker.postMessage({ content, language }); + }, + async highlightHash() { + const { hash } = this.$route; + if (!hash) return; + + // Make the chunk containing the line number visible + const lineNumber = hash.substring(hash.indexOf('L') + 1).split('-')[0]; + const chunkToHighlight = this.chunks.find( + (chunk) => + chunk.startingFrom <= lineNumber && chunk.startingFrom + chunk.totalLines >= lineNumber, + ); + + if (chunkToHighlight) { + chunkToHighlight.isHighlighted = true; + } + + await this.$nextTick(); + const lineHighlighter = new LineHighlighter({ scrollBehavior: 'auto' }); + lineHighlighter.highlightHash(hash); + }, + }, +}; diff --git a/spec/frontend/repository/mixins/highlight_mixin_spec.js b/spec/frontend/repository/mixins/highlight_mixin_spec.js new file mode 100644 index 00000000000000..9805bd2ef8e9d7 --- /dev/null +++ b/spec/frontend/repository/mixins/highlight_mixin_spec.js @@ -0,0 +1,105 @@ +import { shallowMount } from '@vue/test-utils'; +import { splitIntoChunks } from '~/vue_shared/components/source_viewer/workers/highlight_utils'; +import highlightMixin from '~/repository/mixins/highlight_mixin'; +import LineHighlighter from '~/blob/line_highlighter'; +import Tracking from '~/tracking'; +import { TEXT_FILE_TYPE } from '~/repository/constants'; +import { + EVENT_ACTION, + EVENT_LABEL_FALLBACK, + LINES_PER_CHUNK, +} from '~/vue_shared/components/source_viewer/constants'; + +const lineHighlighter = new LineHighlighter(); +jest.mock('~/blob/line_highlighter', () => jest.fn().mockReturnValue({ highlightHash: jest.fn() })); +jest.mock('~/vue_shared/components/source_viewer/workers/highlight_utils', () => ({ + splitIntoChunks: jest.fn().mockResolvedValue([]), +})); + +const workerMock = { postMessage: jest.fn() }; +const onErrorMock = jest.fn(); + +describe('HighlightMixin', () => { + let wrapper; + const hash = '#L50'; + const contentArray = Array.from({ length: 140 }, () => 'newline'); // simulate 140 lines of code + const rawTextBlob = contentArray.join('\n'); + const languageMock = 'javascript'; + + const createComponent = ({ fileType = TEXT_FILE_TYPE, language = languageMock } = {}) => { + const simpleViewer = { fileType }; + + const dummyComponent = { + mixins: [highlightMixin], + inject: { highlightWorker: { default: workerMock } }, + template: '
{{chunks[0]?.highlightedContent}}
', + created() { + this.initHighlightWorker({ rawTextBlob, simpleViewer, language }); + }, + methods: { onError: onErrorMock }, + }; + + wrapper = shallowMount(dummyComponent, { mocks: { $route: { hash } } }); + }; + + beforeEach(() => createComponent()); + + afterEach(() => wrapper.destroy()); + + describe('initHighlightWorker', () => { + const firstSeventyLines = contentArray.slice(0, LINES_PER_CHUNK).join('\n'); + + it('does instruct worker if file is not a text file', () => { + workerMock.postMessage.mockClear(); + createComponent({ fileType: 'markdown' }); + expect(workerMock.postMessage).not.toHaveBeenCalled(); + }); + + it('track event if a language is not supported and does not instruct worker', () => { + const unsupportedLanguage = 'some_unsupported_language'; + const eventData = { label: EVENT_LABEL_FALLBACK, property: unsupportedLanguage }; + + jest.spyOn(Tracking, 'event'); + workerMock.postMessage.mockClear(); + createComponent({ language: unsupportedLanguage }); + + expect(Tracking.event).toHaveBeenCalledWith(undefined, EVENT_ACTION, eventData); + expect(onErrorMock).toHaveBeenCalled(); + expect(workerMock.postMessage).not.toHaveBeenCalled(); + }); + + it('generates a chunk for the first 70 lines of raw text', () => { + expect(splitIntoChunks).toHaveBeenCalledWith(languageMock, firstSeventyLines); + }); + + it('calls postMessage on the worker', () => { + expect(workerMock.postMessage.mock.calls.length).toBe(2); + + // first call instructs worker to highlight the first 70 lines + expect(workerMock.postMessage.mock.calls[0][0]).toMatchObject({ + content: firstSeventyLines, + language: languageMock, + }); + + // second call instructs worker to highlight all of the lines + expect(workerMock.postMessage.mock.calls[1][0]).toMatchObject({ + content: rawTextBlob, + language: languageMock, + }); + }); + }); + + describe('worker message handling', () => { + const CHUNK_MOCK = { startingFrom: 0, totalLines: 70, highlightedContent: 'some content' }; + + beforeEach(() => workerMock.onmessage({ data: [CHUNK_MOCK] })); + + it('updates the chunks data', () => { + expect(wrapper.text()).toBe(CHUNK_MOCK.highlightedContent); + }); + + it('highlights hash', () => { + expect(lineHighlighter.highlightHash).toHaveBeenCalledWith(hash); + }); + }); +}); -- GitLab From 30e602d45275e8cfe32af17bf4ff41af9f154907 Mon Sep 17 00:00:00 2001 From: Jacques Date: Thu, 9 Feb 2023 08:47:47 +0100 Subject: [PATCH 2/3] Address review comments General code review comments --- .../javascripts/repository/mixins/highlight_mixin.js | 11 ++++++----- .../repository/mixins/highlight_mixin_spec.js | 5 +++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/repository/mixins/highlight_mixin.js b/app/assets/javascripts/repository/mixins/highlight_mixin.js index fc1fef724da46d..a9ba663b8bfe32 100644 --- a/app/assets/javascripts/repository/mixins/highlight_mixin.js +++ b/app/assets/javascripts/repository/mixins/highlight_mixin.js @@ -1,3 +1,4 @@ +import Vue from 'vue'; import { LEGACY_FALLBACKS, EVENT_ACTION, @@ -27,11 +28,11 @@ export default { trackEvent(label, language) { this.track(EVENT_ACTION, { label, property: language }); }, - unsupportedLanguage(language) { + isUnsupportedLanguage(language) { const supportedLanguages = Object.keys(languageLoader); - const unsupportedLanguage = !supportedLanguages.includes(language); + const isUnsupportedLanguage = !supportedLanguages.includes(language); - return LEGACY_FALLBACKS.includes(language) || unsupportedLanguage; + return LEGACY_FALLBACKS.includes(language) || isUnsupportedLanguage; }, handleUnsupportedLanguage(language) { this.trackEvent(EVENT_LABEL_FALLBACK, language); @@ -40,7 +41,7 @@ export default { initHighlightWorker({ rawTextBlob, language, simpleViewer }) { if (simpleViewer?.fileType !== TEXT_FILE_TYPE) return; - if (this.unsupportedLanguage(language)) { + if (this.isUnsupportedLanguage(language)) { this.handleUnsupportedLanguage(language); return; } @@ -80,7 +81,7 @@ export default { chunkToHighlight.isHighlighted = true; } - await this.$nextTick(); + await Vue.nextTick(); const lineHighlighter = new LineHighlighter({ scrollBehavior: 'auto' }); lineHighlighter.highlightHash(hash); }, diff --git a/spec/frontend/repository/mixins/highlight_mixin_spec.js b/spec/frontend/repository/mixins/highlight_mixin_spec.js index 9805bd2ef8e9d7..7c48fe440d2a32 100644 --- a/spec/frontend/repository/mixins/highlight_mixin_spec.js +++ b/spec/frontend/repository/mixins/highlight_mixin_spec.js @@ -49,13 +49,14 @@ describe('HighlightMixin', () => { describe('initHighlightWorker', () => { const firstSeventyLines = contentArray.slice(0, LINES_PER_CHUNK).join('\n'); - it('does instruct worker if file is not a text file', () => { + it('does not instruct worker if file is not a text file', () => { workerMock.postMessage.mockClear(); createComponent({ fileType: 'markdown' }); + expect(workerMock.postMessage).not.toHaveBeenCalled(); }); - it('track event if a language is not supported and does not instruct worker', () => { + it('tracks event if a language is not supported and does not instruct worker', () => { const unsupportedLanguage = 'some_unsupported_language'; const eventData = { label: EVENT_LABEL_FALLBACK, property: unsupportedLanguage }; -- GitLab From c60ed8ef8ce6388e5f223b06fa577764b6dbaa30 Mon Sep 17 00:00:00 2001 From: Jacques Date: Tue, 14 Feb 2023 11:43:01 +0100 Subject: [PATCH 3/3] Address code review comments General code review comments --- .../repository/mixins/highlight_mixin.js | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/repository/mixins/highlight_mixin.js b/app/assets/javascripts/repository/mixins/highlight_mixin.js index a9ba663b8bfe32..95d0c55bb0406c 100644 --- a/app/assets/javascripts/repository/mixins/highlight_mixin.js +++ b/app/assets/javascripts/repository/mixins/highlight_mixin.js @@ -1,4 +1,4 @@ -import Vue from 'vue'; +import { nextTick } from 'vue'; import { LEGACY_FALLBACKS, EVENT_ACTION, @@ -36,7 +36,7 @@ export default { }, handleUnsupportedLanguage(language) { this.trackEvent(EVENT_LABEL_FALLBACK, language); - this.onError(); + this?.onError(); }, initHighlightWorker({ rawTextBlob, language, simpleViewer }) { if (simpleViewer?.fileType !== TEXT_FILE_TYPE) return; @@ -46,6 +46,21 @@ export default { return; } + /* + * We want to start rendering content as soon as possible, but highlighting large amounts of + * content can take long, so we render the content in phases: + * + * 1. `splitIntoChunks` with the first 70 lines of raw text. + * This ensures that we start rendering raw content in the DOM as soon as we can so that + * the user can see content as fast as possible (improves perceived performance and LCP). + * 2. `instructWorker` to start highlighting the first 70 lines. + * This ensures that we display highlighted** content to the user as fast as possible + * (improves perceived performance and makes the first 70 lines look nice). + * 3. `instructWorker` to start highlighting all the content. + * This is the longest task. It ensures that we highlight all content, since the first 70 + * lines are already rendered, this can happen in the background. + */ + // Render the first 70 lines (raw text) ASAP, this improves perceived performance and LCP. const firstSeventyLines = rawTextBlob.split(/\r?\n/).slice(0, LINES_PER_CHUNK).join('\n'); @@ -61,7 +76,7 @@ export default { }, handleWorkerMessage({ data }) { this.chunks = data; - this.highlightHash(); // highlight the line is a line number hash is present in the URL + this.highlightHash(); // highlight the line if a line number hash is present in the URL }, instructWorker(content, language) { this.highlightWorker.postMessage({ content, language }); @@ -81,7 +96,9 @@ export default { chunkToHighlight.isHighlighted = true; } - await Vue.nextTick(); + // Line numbers in the DOM needs to update first based on changes made to `chunks`. + await nextTick(); + const lineHighlighter = new LineHighlighter({ scrollBehavior: 'auto' }); lineHighlighter.highlightHash(hash); }, -- GitLab