From f7e909f58eedd02e548e25925e5fae1559d24030 Mon Sep 17 00:00:00 2001 From: Stanislav Lashmanov Date: Mon, 7 Feb 2022 06:07:37 +0300 Subject: [PATCH 1/4] POC: Diffs streaming --- app/assets/javascripts/actioncable_link.js | 4 +- .../behaviors/markdown/copy_as_gfm.js | 2 +- .../javascripts/diffs/components/app.vue | 189 ++++++---- .../diffs/components/batch_fetcher.vue | 61 ++++ .../components/compare_dropdown_layout.vue | 1 + .../diffs/components/compare_versions.vue | 5 +- .../diffs/components/diff_content.vue | 3 +- .../diffs/components/diff_file.vue | 4 +- .../diffs/components/diff_file_header.vue | 4 +- .../diffs/components/diff_view.vue | 2 +- .../diffs/components/no_changes.vue | 1 + .../diffs/components/settings_dropdown.vue | 1 + .../diffs/components/tree_list.vue | 2 +- app/assets/javascripts/diffs/index.js | 140 ++++---- app/assets/javascripts/diffs/store/actions.js | 79 +++-- app/assets/javascripts/diffs/store/getters.js | 1 + app/assets/javascripts/diffs/streaming.js | 32 ++ .../javascripts/diffs/utils/file_reviews.js | 1 + app/assets/javascripts/dompurify.js | 3 + app/assets/javascripts/entry-server.js | 6 + app/assets/javascripts/gl_form.js | 9 +- app/assets/javascripts/lib/dompurify.js | 84 ++--- app/assets/javascripts/lib/graphql.js | 36 +- app/assets/javascripts/lib/utils/accessor.js | 1 + .../lib/utils/axios_startup_calls.js | 4 +- .../javascripts/lib/utils/axios_utils.js | 3 + .../javascripts/lib/utils/color_utils.js | 4 +- app/assets/javascripts/lib/utils/csrf.js | 4 + .../lib/utils/is_navigating_away.js | 6 +- .../javascripts/lib/utils/jquery_at_who.js | 6 +- .../javascripts/lib/utils/url_utility.js | 10 +- app/assets/javascripts/locale/index.js | 13 +- app/assets/javascripts/mr_notes/index.js | 6 +- .../javascripts/mr_notes/stores/index.js | 4 +- .../mr_notes/stores/modules/index.js | 4 +- .../mr_popover/components/mr_popover.vue | 2 +- .../notes/components/note_body.vue | 4 +- .../javascripts/notes/stores/actions.js | 26 +- app/assets/javascripts/pikaday.js | 13 + .../commit_pipeline_status_component.vue | 5 +- .../sidebar_confidentiality_widget.vue | 8 +- app/assets/javascripts/sidebar/event_hub.js | 2 +- app/assets/javascripts/task_list.js | 4 +- app/assets/javascripts/user_popovers.js | 15 +- .../vue_shared/components/markdown/field.vue | 4 +- .../components/user_popover/user_popover.vue | 2 +- app/assets/stylesheets/framework/diffs.scss | 3 + .../merge_requests/diffs_controller.rb | 3 + app/helpers/merge_requests_helper.rb | 17 +- config/webpack.config.js | 4 +- config/webpack.ssr.config.js | 226 ++++++++++++ db/structure.sql | 2 +- .../assets/javascripts/diffs/store/actions.js | 25 +- .../content_security_policy/config_loader.rb | 1 + package.json | 8 +- server.js | 36 ++ test.http | 3 + yarn.lock | 333 +++++++++++++++++- 58 files changed, 1164 insertions(+), 317 deletions(-) create mode 100644 app/assets/javascripts/diffs/components/batch_fetcher.vue create mode 100644 app/assets/javascripts/diffs/streaming.js create mode 100644 app/assets/javascripts/dompurify.js create mode 100644 app/assets/javascripts/entry-server.js create mode 100644 app/assets/javascripts/pikaday.js create mode 100644 config/webpack.ssr.config.js create mode 100644 server.js create mode 100644 test.http diff --git a/app/assets/javascripts/actioncable_link.js b/app/assets/javascripts/actioncable_link.js index 895a34ba157052..af9007e0b2eb01 100644 --- a/app/assets/javascripts/actioncable_link.js +++ b/app/assets/javascripts/actioncable_link.js @@ -1,12 +1,12 @@ import { ApolloLink, Observable } from 'apollo-link'; import { print } from 'graphql'; -import cable from '~/actioncable_consumer'; import { uuids } from '~/lib/utils/uuids'; export default class ActionCableLink extends ApolloLink { // eslint-disable-next-line class-methods-use-this request(operation) { - return new Observable((observer) => { + return new Observable(async (observer) => { + const cable = await import('~/actioncable_consumer'); const subscription = cable.subscriptions.create( { channel: 'GraphqlChannel', diff --git a/app/assets/javascripts/behaviors/markdown/copy_as_gfm.js b/app/assets/javascripts/behaviors/markdown/copy_as_gfm.js index 19ebab36481cc9..0a7a3a287572c8 100644 --- a/app/assets/javascripts/behaviors/markdown/copy_as_gfm.js +++ b/app/assets/javascripts/behaviors/markdown/copy_as_gfm.js @@ -189,7 +189,7 @@ export class CopyAsGFM { // Export CopyAsGFM as a global for rspec to access // see /spec/features/markdown/copy_as_gfm_spec.rb -if (process.env.NODE_ENV !== 'production') { +if (process.env.NODE_ENV !== 'production' && typeof window !== 'undefined') { window.CopyAsGFM = CopyAsGFM; } diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index a8ca17ab4dd590..68d3cfe61d5059 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -11,7 +11,7 @@ import { MR_COMMITS_NEXT_COMMIT, MR_COMMITS_PREVIOUS_COMMIT, } from '~/behaviors/shortcuts/keybindings'; -import createFlash from '~/flash'; +import BatchFetcher from "~/diffs/components/batch_fetcher.vue"; import { isSingleViewStyle } from '~/helpers/diffs_helper'; import { helpPagePath } from '~/helpers/help_page_helper'; import { parseBoolean } from '~/lib/utils/common_utils'; @@ -57,6 +57,7 @@ import TreeList from './tree_list.vue'; export default { name: 'DiffsApp', components: { + BatchFetcher, DynamicScroller: () => import('vendor/vue-virtual-scroller').then(({ DynamicScroller }) => DynamicScroller), DynamicScrollerItem: () => @@ -78,6 +79,26 @@ export default { MrWidgetHowToMergeModal, GlAlert, }, + async serverPrefetch() { + this.setBaseConfig({ + endpoint: this.endpoint, + endpointMetadata: this.endpointMetadata, + endpointBatch: this.endpointBatch, + endpointCoverage: this.endpointCoverage, + endpointUpdateUser: this.endpointUpdateUser, + projectPath: this.projectPath, + dismissEndpoint: this.dismissEndpoint, + showSuggestPopover: this.showSuggestPopover, + viewDiffsFileByFile: this.fileByFileUserPreference || false, + defaultSuggestionCommitMessage: this.defaultSuggestionCommitMessage, + mrReviews: this.rehydratedMrReviews, + }); + try { + await this.fetchData(); + } catch (error) { + console.log(error); + } + }, mixins: [glFeatureFlagsMixin()], alerts: { ALERT_OVERFLOW_HIDDEN, @@ -182,8 +203,8 @@ export default { }, }, data() { - const treeWidth = - parseInt(localStorage.getItem(TREE_LIST_WIDTH_STORAGE_KEY), 10) || INITIAL_TREE_WIDTH; + const treeWidth = typeof window !== 'undefined' ? + parseInt(localStorage.getItem(TREE_LIST_WIDTH_STORAGE_KEY), 10) || INITIAL_TREE_WIDTH : INITIAL_TREE_WIDTH; return { treeWidth, @@ -239,10 +260,18 @@ export default { return file.file_hash === this.currentDiffFileId || (i === 0 && !this.currentDiffFileId); }); }, + perPage() { + return 5; + return this.diffFilesLength ? 1 : 5; + }, + perPageLength() { + return Math.floor(this.diffFilesLength / this.perPage); + }, canCurrentUserFork() { return this.currentUser.can_fork === true && this.currentUser.can_create_merge_request; }, renderDiffFiles() { + return this.diffFilesLength; return this.diffFiles.length > 0; }, renderFileTree() { @@ -319,19 +348,23 @@ export default { renderFileTree: 'adjustView', }, mounted() { - this.setBaseConfig({ - endpoint: this.endpoint, - endpointMetadata: this.endpointMetadata, - endpointBatch: this.endpointBatch, - endpointCoverage: this.endpointCoverage, - endpointUpdateUser: this.endpointUpdateUser, - projectPath: this.projectPath, - dismissEndpoint: this.dismissEndpoint, - showSuggestPopover: this.showSuggestPopover, - viewDiffsFileByFile: this.fileByFileUserPreference || false, - defaultSuggestionCommitMessage: this.defaultSuggestionCommitMessage, - mrReviews: this.rehydratedMrReviews, - }); + this.adjustView(); + this.subscribeToEvents(); + + this.unwatchDiscussions = this.$watch( + () => `${this.diffFiles.length}:${this.$store.state.notes.discussions.length}`, + () => this.setDiscussions(), + ); + + this.unwatchRetrievingBatches = this.$watch( + () => `${this.retrievingBatches}:${this.$store.state.notes.discussions.length}`, + () => { + if (!this.retrievingBatches && this.$store.state.notes.discussions.length) { + this.unwatchDiscussions(); + this.unwatchRetrievingBatches(); + } + }, + ); if (this.endpointCodequality) { this.setCodequalityEndpoint(this.endpointCodequality); @@ -380,26 +413,11 @@ export default { this.subscribeToVirtualScrollingEvents(); }, beforeCreate() { + if (typeof window === 'undefined') return; diffsApp.instrument(); }, - created() { - this.adjustView(); - this.subscribeToEvents(); - - this.unwatchDiscussions = this.$watch( - () => `${this.diffFiles.length}:${this.$store.state.notes.discussions.length}`, - () => this.setDiscussions(), - ); - - this.unwatchRetrievingBatches = this.$watch( - () => `${this.retrievingBatches}:${this.$store.state.notes.discussions.length}`, - () => { - if (!this.retrievingBatches && this.$store.state.notes.discussions.length) { - this.unwatchDiscussions(); - this.unwatchRetrievingBatches(); - } - }, - ); + errorCaptured(err, vm, info) { + console.log(err, info); }, beforeDestroy() { diffsApp.deinstrument(); @@ -451,43 +469,47 @@ export default { needsFirstLoad() { return !this.diffFiles.length; }, - fetchData(toggleTree = true) { - this.fetchDiffFilesMeta() - .then(({ real_size }) => { + async fetchData(toggleTree = true) { + let real_size; + try { + [ + { real_size }, + ] = await Promise.all( + [ + this.fetchDiffFilesMeta(), + // this.fetchDiffFilesBatch(), + ] + ); + } catch(error) { + console.log(error); + } + + + this.diffFilesLength = parseInt(real_size, 10); - if (toggleTree) this.setTreeDisplay(); - }) - .catch(() => { - createFlash({ - message: __('Something went wrong on our end. Please try again!'), - }); - }); - this.fetchDiffFilesBatch() - .then(() => { - if (toggleTree) this.setTreeDisplay(); - // Guarantee the discussions are assigned after the batch finishes. - // Just watching the length of the discussions or the diff files - // isn't enough, because with split diff loading, neither will - // change when loading the other half of the diff files. - this.setDiscussions(); - }) - .catch(() => { - createFlash({ - message: __('Something went wrong on our end. Please try again!'), - }); - }); + if (typeof window !== 'undefined') { + if (toggleTree) this.setTreeDisplay(); - if (this.endpointCoverage) { - this.fetchCoverageFiles(); - } - if (this.endpointCodequality) { - this.fetchCodequality(); - } + // Guarantee the discussions are assigned after the batch finishes. + // Just watching the length of the discussions or the diff files + // isn't enough, because with split diff loading, neither will + // change when loading the other half of the diff files. + this.setDiscussions(); - if (!this.isNotesFetched) { - notesEventHub.$emit('fetchNotesData'); + + if (this.endpointCoverage) { + this.fetchCoverageFiles(); + } + + if (this.endpointCodequality) { + this.fetchCodequality(); + } + + if (!this.isNotesFetched) { + notesEventHub.$emit('fetchNotesData'); + } } }, setDiscussions() { @@ -510,6 +532,7 @@ export default { } }, setEventListeners() { + if (typeof window === 'undefined') return; Mousetrap.bind(keysFor(MR_PREVIOUS_FILE_IN_DIFF), () => this.jumpToFile(-1)); Mousetrap.bind(keysFor(MR_NEXT_FILE_IN_DIFF), () => this.jumpToFile(+1)); @@ -550,6 +573,7 @@ export default { } }, removeEventListeners() { + if (typeof window === 'undefined') return; Mousetrap.unbind(keysFor(MR_PREVIOUS_FILE_IN_DIFF)); Mousetrap.unbind(keysFor(MR_NEXT_FILE_IN_DIFF)); Mousetrap.unbind(keysFor(MR_COMMITS_NEXT_COMMIT)); @@ -621,7 +645,7 @@ export default { }, }, minTreeWidth: MIN_TREE_WIDTH, - maxTreeWidth: window.innerWidth / 2, + maxTreeWidth: typeof window !== 'undefined' ? window.innerWidth / 2 : 0, howToMergeDocsPath: helpPagePath('user/project/merge_requests/reviews/index.md', { anchor: 'checkout-merge-requests-locally-through-the-head-ref', }), @@ -722,17 +746,26 @@ export default {
+import axios from "~/lib/utils/axios_utils.js"; +import { mergeUrlParams } from "~/lib/utils/url_utility.js"; + +export default { + name: 'BatchFetcher', + props: { + endpoint: { + type: String, + required: true, + }, + page: { + type: Number, + required: true, + }, + perPage: { + type: Number, + required: true, + }, + showWhitespace: { + type: Boolean, + required: true, + }, + }, + async serverPrefetch() { + await this.fetchFilesBatch(); + }, + data() { + return { + filesBatch: null, + } + }, + methods: { + async fetchFilesBatch() { + let baseUrl = ''; + let headers = {}; + if (typeof window === 'undefined') { + baseUrl = process.env.BASE_URL; + headers = { cookie: process.env.COOKIE }; + } + const urlParams = { + w: this.showWhitespace ? '0' : '1', + view: 'inline', + }; + const { data: { diff_files: filesBatch } } = await axios + .get( + `${baseUrl}${mergeUrlParams({ ...urlParams, page: this.page, per_page: this.perPage }, this.endpoint)}`, + { + headers + }); + this.filesBatch = filesBatch; + }, + }, +} + + + diff --git a/app/assets/javascripts/diffs/components/compare_dropdown_layout.vue b/app/assets/javascripts/diffs/components/compare_dropdown_layout.vue index 6c5973b7c28dda..b4572200cb076a 100644 --- a/app/assets/javascripts/diffs/components/compare_dropdown_layout.vue +++ b/app/assets/javascripts/diffs/components/compare_dropdown_layout.vue @@ -1,6 +1,7 @@ diff --git a/app/assets/javascripts/diffs/components/diff_content.vue b/app/assets/javascripts/diffs/components/diff_content.vue index 858d9e221aea96..3c10b9e93fdc57 100644 --- a/app/assets/javascripts/diffs/components/diff_content.vue +++ b/app/assets/javascripts/diffs/components/diff_content.vue @@ -71,6 +71,7 @@ export default { return this.getCommentFormForDiffFile(this.diffFileHash); }, showNotesContainer() { + return false; return this.imageDiscussions.length || this.diffFileCommentForm; }, diffFileHash() { @@ -142,7 +143,7 @@ export default { >