Merge branch 'ps-step-3-use-ide-files-change-to-update-clientside-preview' into 'master'

Step 3 - Use ide.files.change for live preview

See merge request gitlab-org/gitlab!50255

app/assets/javascripts/ide/components/preview/clientside.vue

 <script>
 import { mapActions, mapGetters, mapState } from 'vuex';
-import { isEmpty } from 'lodash';
+import { isEmpty, debounce } from 'lodash';
 import { Manager } from 'smooshpack';
 import { listen } from 'codesandbox-api';
 import { GlLoadingIcon } from '@gitlab/ui';
 import Navigator from './navigator.vue';
-import { packageJsonPath } from '../../constants';
+import { packageJsonPath, LIVE_PREVIEW_DEBOUNCE } from '../../constants';
 import { createPathWithExt } from '../../utils';
+import eventHub from '../../eventhub';
 
 export default {
   components: {
@@ -61,13 +62,10 @@ export default {
       };
     },
   },
-  watch: {
-    entries: {
-      deep: true,
-      handler: 'update',
-    },
-  },
   mounted() {
+    this.onFilesChangeCallback = debounce(() => this.update(), LIVE_PREVIEW_DEBOUNCE);
+    eventHub.$on('ide.files.change', this.onFilesChangeCallback);
+
     this.loading = true;
 
     return this.loadFileContent(packageJsonPath)
@@ -78,17 +76,19 @@ export default {
       .then(() => this.initPreview());
   },
   beforeDestroy() {
+    // Setting sandpackReady = false protects us form a phantom `update()` being called when `debounce` finishes.
+    this.sandpackReady = false;
+    eventHub.$off('ide.files.change', this.onFilesChangeCallback);
+
     if (!isEmpty(this.manager)) {
       this.manager.listener();
     }
+
     this.manager = {};
 
     if (this.listener) {
       this.listener();
     }
-
-    clearTimeout(this.timeout);
-    this.timeout = null;
   },
   methods: {
     ...mapActions(['getFileData', 'getRawFileData']),
@@ -122,17 +122,13 @@ export default {
     update() {
       if (!this.sandpackReady) return;
 
-      clearTimeout(this.timeout);
-
-      this.timeout = setTimeout(() => {
-        if (isEmpty(this.manager)) {
-          this.initPreview();
+      if (isEmpty(this.manager)) {
+        this.initPreview();
 
-          return;
-        }
+        return;
+      }
 
-        this.manager.updatePreview(this.sandboxOpts);
-      }, 250);
+      this.manager.updatePreview(this.sandboxOpts);
     },
     initManager() {
       const { codesandboxBundlerUrl: bundlerURL } = this;

app/assets/javascripts/ide/constants.js

@@ -97,3 +97,6 @@ export const packageJsonPath = 'package.json';
 
 export const SIDE_LEFT = 'left';
 export const SIDE_RIGHT = 'right';
+
+// Live Preview feature
+export const LIVE_PREVIEW_DEBOUNCE = 2000;

app/assets/javascripts/ide/stores/actions/file.js

@@ -10,7 +10,7 @@ import eventHub from '../../eventhub';
 import service from '../../services';
 import * as types from '../mutation_types';
 import { setPageTitleForFile } from '../utils';
-import { viewerTypes, stageKeys } from '../../constants';
+import { viewerTypes, stageKeys, commitActionTypes } from '../../constants';
 
 export const closeFile = ({ commit, state, dispatch, getters }, file) => {
   const { path } = file;
@@ -164,7 +164,7 @@ export const getRawFileData = ({ state, commit, dispatch, getters }, { path }) =
     });
 };
 
-export const changeFileContent = ({ commit, state, getters }, { path, content }) => {
+export const changeFileContent = ({ commit, dispatch, state, getters }, { path, content }) => {
   const file = state.entries[path];
 
   // It's possible for monaco to hit a race condition where it tries to update renamed files.
@@ -185,6 +185,8 @@ export const changeFileContent = ({ commit, state, getters }, { path, content })
   } else if (!file.changed && !file.tempFile && indexOfChangedFile !== -1) {
     commit(types.REMOVE_FILE_FROM_CHANGED, path);
   }
+
+  dispatch('triggerFilesChange', { type: commitActionTypes.update, path });
 };
 
 export const restoreOriginalFile = ({ dispatch, state, commit }, path) => {

app/assets/javascripts/ide/stores/modules/editor/setup.js

@@ -9,10 +9,15 @@ const removeUnusedFileEditors = (store) => {
 
 export const setupFileEditorsSync = (store) => {
   eventHub.$on('ide.files.change', ({ type, ...payload } = {}) => {
+    // Do nothing on file update because the file tree itself hasn't changed.
+    if (type === commitActionTypes.update) {
+      return;
+    }
+
     if (type === commitActionTypes.move) {
       store.dispatch('editor/renameFileEditor', payload);
     } else {
-      // The files have changed, but the specific change is not known.
+      // The file tree has changed, but the specific change is not known.
       removeUnusedFileEditors(store);
     }
   });

app/assets/javascripts/ide/stores/plugins/terminal_sync.js

 import { debounce } from 'lodash';
 import eventHub from '~/ide/eventhub';
+import { commitActionTypes } from '~/ide/constants';
 import terminalSyncModule from '../modules/terminal_sync';
 import { isEndingStatus, isRunningStatus } from '../modules/terminal/utils';
 
@@ -19,16 +20,25 @@ export default function createMirrorPlugin() {
       store.dispatch(`terminalSync/upload`);
     }, UPLOAD_DEBOUNCE);
 
+    const onFilesChange = (payload) => {
+      // Do nothing on a file update since we only want to trigger manually on "save".
+      if (payload?.type === commitActionTypes.update) {
+        return;
+      }
+
+      upload();
+    };
+
     const stop = () => {
       store.dispatch(`terminalSync/stop`);
-      eventHub.$off('ide.files.change', upload);
+      eventHub.$off('ide.files.change', onFilesChange);
     };
 
     const start = () => {
       store
         .dispatch(`terminalSync/start`)
         .then(() => {
-          eventHub.$on('ide.files.change', upload);
+          eventHub.$on('ide.files.change', onFilesChange);
         })
         .catch(() => {
           // error is handled in store

changelogs/unreleased/ps-step-3-use-ide-files-change-to-update-clientside-preview.yml

+---
+title: Fix over-eagerly updating Web IDE Live Preview
+merge_request: 50255
+author:
+type: fixed

spec/frontend/ide/components/preview/clientside_spec.js

@@ -3,6 +3,7 @@ import { GlLoadingIcon } from '@gitlab/ui';
 import { shallowMount, createLocalVue } from '@vue/test-utils';
 import smooshpack from 'smooshpack';
 import Clientside from '~/ide/components/preview/clientside.vue';
+import eventHub from '~/ide/eventhub';
 
 jest.mock('smooshpack', () => ({
   Manager: jest.fn(),
@@ -70,6 +71,17 @@ describe('IDE clientside preview', () => {
     });
   };
 
+  const createInitializedComponent = () => {
+    createComponent();
+    wrapper.setData({
+      sandpackReady: true,
+      manager: {
+        listener: jest.fn(),
+        updatePreview: jest.fn(),
+      },
+    });
+  };
+
   afterEach(() => {
     wrapper.destroy();
   });
@@ -293,33 +305,33 @@ describe('IDE clientside preview', () => {
     });
 
     describe('update', () => {
-      beforeEach(() => {
-        createComponent();
-        wrapper.setData({ sandpackReady: true });
-      });
-
       it('initializes manager if manager is empty', () => {
         createComponent({ getters: { packageJson: dummyPackageJson } });
         wrapper.setData({ sandpackReady: true });
         wrapper.vm.update();
 
-        jest.advanceTimersByTime(250);
-
         return waitForCalls().then(() => {
           expect(smooshpack.Manager).toHaveBeenCalled();
         });
       });
 
       it('calls updatePreview', () => {
-        wrapper.setData({
-          manager: {
-            listener: jest.fn(),
-            updatePreview: jest.fn(),
-          },
-        });
+        createInitializedComponent();
+
         wrapper.vm.update();
 
-        jest.advanceTimersByTime(250);
+        expect(wrapper.vm.manager.updatePreview).toHaveBeenCalledWith(wrapper.vm.sandboxOpts);
+      });
+    });
+
+    describe('on ide.files.change event', () => {
+      beforeEach(() => {
+        createInitializedComponent();
+
+        eventHub.$emit('ide.files.change');
+      });
+
+      it('calls updatePreview', () => {
         expect(wrapper.vm.manager.updatePreview).toHaveBeenCalledWith(wrapper.vm.sandboxOpts);
       });
     });
@@ -355,4 +367,18 @@ describe('IDE clientside preview', () => {
       });
     });
   });
+
+  describe('when destroyed', () => {
+    let spy;
+
+    beforeEach(() => {
+      createInitializedComponent();
+      spy = wrapper.vm.manager.updatePreview;
+      wrapper.destroy();
+    });
+
+    it('does not call updatePreview', () => {
+      expect(spy).not.toHaveBeenCalled();
+    });
+  });
 });

spec/frontend/ide/helpers.js

@@ -41,5 +41,10 @@ export const createTriggerRenamePayload = (path, newPath) => ({
   newPath,
 });
 
+export const createTriggerUpdatePayload = (path) => ({
+  type: commitActionTypes.update,
+  path,
+});
+
 export const createTriggerRenameAction = (path, newPath) =>
   createTriggerChangeAction(createTriggerRenamePayload(path, newPath));

spec/frontend/ide/stores/actions/file_spec.js

@@ -7,7 +7,7 @@ import * as types from '~/ide/stores/mutation_types';
 import service from '~/ide/services';
 import { createRouter } from '~/ide/ide_router';
 import eventHub from '~/ide/eventhub';
-import { file, createTriggerRenameAction } from '../../helpers';
+import { file, createTriggerRenameAction, createTriggerUpdatePayload } from '../../helpers';
 
 const ORIGINAL_CONTENT = 'original content';
 const RELATIVE_URL_ROOT = '/gitlab';
@@ -510,12 +510,15 @@ describe('IDE store file actions', () => {
 
   describe('changeFileContent', () => {
     let tmpFile;
+    let onFilesChange;
 
     beforeEach(() => {
       tmpFile = file('tmpFile');
       tmpFile.content = '\n';
       tmpFile.raw = '\n';
       store.state.entries[tmpFile.path] = tmpFile;
+      onFilesChange = jest.fn();
+      eventHub.$on('ide.files.change', onFilesChange);
     });
 
     it('updates file content', () => {
@@ -580,6 +583,17 @@ describe('IDE store file actions', () => {
           expect(store.state.changedFiles.length).toBe(0);
         });
     });
+
+    it('triggers ide.files.change', async () => {
+      expect(onFilesChange).not.toHaveBeenCalled();
+
+      await store.dispatch('changeFileContent', {
+        path: tmpFile.path,
+        content: 'content\n',
+      });
+
+      expect(onFilesChange).toHaveBeenCalledWith(createTriggerUpdatePayload(tmpFile.path));
+    });
   });
 
   describe('with changed file', () => {

spec/frontend/ide/stores/modules/editor/setup_spec.js

+import { cloneDeep } from 'lodash';
 import Vuex from 'vuex';
 import eventHub from '~/ide/eventhub';
 import { createStoreOptions } from '~/ide/stores';
 import { setupFileEditorsSync } from '~/ide/stores/modules/editor/setup';
-import { createTriggerRenamePayload } from '../../../helpers';
+import { createTriggerRenamePayload, createTriggerUpdatePayload } from '../../../helpers';
 
 describe('~/ide/stores/modules/editor/setup', () => {
   let store;
@@ -33,6 +34,14 @@ describe('~/ide/stores/modules/editor/setup', () => {
     });
   });
 
+  it('when files update is emitted, does nothing', () => {
+    const origState = cloneDeep(store.state);
+
+    eventHub.$emit('ide.files.change', createTriggerUpdatePayload('foo'));
+
+    expect(store.state).toEqual(origState);
+  });
+
   it('when files rename is emitted, renames fileEditor', () => {
     eventHub.$emit('ide.files.change', createTriggerRenamePayload('foo', 'foo_new'));
 

spec/frontend/ide/stores/plugins/terminal_sync_spec.js

@@ -4,6 +4,7 @@ import { SET_SESSION_STATUS } from '~/ide/stores/modules/terminal/mutation_types
 import { RUNNING, STOPPING } from '~/ide/stores/modules/terminal/constants';
 import { createStore } from '~/ide/stores';
 import eventHub from '~/ide/eventhub';
+import { createTriggerUpdatePayload } from '../../helpers';
 
 jest.mock('~/ide/lib/mirror');
 
@@ -51,6 +52,14 @@ describe('IDE stores/plugins/mirror', () => {
       expect(store.dispatch).toHaveBeenCalledWith(ACTION_UPLOAD);
     });
 
+    it('does nothing when ide.files.change is emitted with "update"', () => {
+      eventHub.$emit(FILES_CHANGE_EVENT, createTriggerUpdatePayload('foo'));
+
+      jest.runAllTimers();
+
+      expect(store.dispatch).not.toHaveBeenCalledWith(ACTION_UPLOAD);
+    });
+
     describe('when session stops', () => {
       beforeEach(() => {
         store.commit(`terminal/${SET_SESSION_STATUS}`, STOPPING);