Merge branch '208800-step-1-1-create-sync-store-and-router' into 'master'

Step 1.1 - Create modules to help break store and router cyclical dependency

See merge request gitlab-org/gitlab!33787

app/assets/javascripts/ide/components/branches/item.vue

@@ -2,7 +2,6 @@
 /* eslint-disable @gitlab/vue-require-i18n-strings */
 import Icon from '~/vue_shared/components/icon.vue';
 import Timeago from '~/vue_shared/components/time_ago_tooltip.vue';
-import router from '../../ide_router';
 
 export default {
   components: {
@@ -26,7 +25,7 @@ export default {
   },
   computed: {
     branchHref() {
-      return router.resolve(`/project/${this.projectId}/edit/${this.item.name}`).href;
+      return this.$router.resolve(`/project/${this.projectId}/edit/${this.item.name}`).href;
     },
   },
 };

app/assets/javascripts/ide/components/merge_requests/item.vue

 <script>
 import Icon from '../../../vue_shared/components/icon.vue';
-import router from '../../ide_router';
 
 export default {
   components: {
@@ -33,7 +32,7 @@ export default {
     mergeRequestHref() {
       const path = `/project/${this.item.projectPathWithNamespace}/merge_requests/${this.item.iid}`;
 
-      return router.resolve(path).href;
+      return this.$router.resolve(path).href;
     },
   },
 };

app/assets/javascripts/ide/components/repo_tabs.vue

 <script>
 import { mapActions } from 'vuex';
 import RepoTab from './repo_tab.vue';
-import router from '../ide_router';
 
 export default {
   components: {
@@ -28,7 +27,7 @@ export default {
 
       if (this.activeFile.pending) {
         return this.removePendingTab(this.activeFile).then(() => {
-          router.push(`/project${this.activeFile.url}`);
+          this.$router.push(`/project${this.activeFile.url}`);
         });
       }
 

app/assets/javascripts/ide/stores/modules/router/actions.js

+import * as types from './mutation_types';
+
+// eslint-disable-next-line import/prefer-default-export
+export const push = ({ commit }, fullPath) => {
+  commit(types.PUSH, fullPath);
+};

app/assets/javascripts/ide/stores/modules/router/index.js

+import state from './state';
+import mutations from './mutations';
+import * as actions from './actions';
+
+export default {
+  namespaced: true,
+  state,
+  mutations,
+  actions,
+};

app/assets/javascripts/ide/stores/modules/router/mutation_types.js

+// eslint-disable-next-line import/prefer-default-export
+export const PUSH = 'PUSH';

app/assets/javascripts/ide/stores/modules/router/mutations.js

+import * as types from './mutation_types';
+
+export default {
+  [types.PUSH](state, fullPath) {
+    state.fullPath = fullPath;
+  },
+};

app/assets/javascripts/ide/stores/modules/router/state.js

+export default () => ({
+  fullPath: '',
+});

app/assets/javascripts/ide/sync_router_and_store.js

+/* eslint-disable import/prefer-default-export */
+/**
+ * This method adds listeners to the given router and store and syncs their state with eachother
+ *
+ * ### Why?
+ *
+ * Previously the IDE had a circular dependency between a singleton router and a singleton store.
+ * This causes some integration testing headaches...
+ *
+ * At the time, the most effecient way to break this ciruclar dependency was to:
+ *
+ * - Replace the router with a factory function that receives a store reference
+ * - Have the store write to a certain state that can be watched by the router
+ *
+ * Hence... This helper function...
+ */
+export const syncRouterAndStore = (router, store) => {
+  const disposables = [];
+
+  let currentPath = '';
+
+  // sync store to router
+  disposables.push(
+    store.watch(
+      state => state.router.fullPath,
+      fullPath => {
+        if (currentPath === fullPath) {
+          return;
+        }
+
+        currentPath = fullPath;
+
+        router.push(fullPath);
+      },
+    ),
+  );
+
+  // sync router to store
+  disposables.push(
+    router.afterEach(to => {
+      if (currentPath === to.fullPath) {
+        return;
+      }
+
+      currentPath = to.fullPath;
+      store.dispatch('router/push', currentPath, { root: true });
+    }),
+  );
+
+  const unsync = () => {
+    disposables.forEach(fn => fn());
+  };
+
+  return unsync;
+};

spec/frontend/ide/components/branches/item_spec.js

@@ -22,6 +22,7 @@ describe('IDE branch item', () => {
         isActive: false,
         ...props,
       },
+      router,
     });
   }
 

spec/frontend/ide/components/merge_requests/item_spec.js

-import Vue from 'vue';
+import { mount } from '@vue/test-utils';
 import router from '~/ide/ide_router';
 import Item from '~/ide/components/merge_requests/item.vue';
-import mountCompontent from '../../../helpers/vue_mount_component_helper';
+
+const TEST_ITEM = {
+  iid: 1,
+  projectPathWithNamespace: 'gitlab-org/gitlab-ce',
+  title: 'Merge request title',
+};
 
 describe('IDE merge request item', () => {
-  const Component = Vue.extend(Item);
-  let vm;
-
-  beforeEach(() => {
-    vm = mountCompontent(Component, {
-      item: {
-        iid: 1,
-        projectPathWithNamespace: 'gitlab-org/gitlab-ce',
-        title: 'Merge request title',
+  let wrapper;
+
+  const createComponent = (props = {}) => {
+    wrapper = mount(Item, {
+      propsData: {
+        item: {
+          ...TEST_ITEM,
+        },
+        currentId: `${TEST_ITEM.iid}`,
+        currentProjectId: TEST_ITEM.projectPathWithNamespace,
+        ...props,
       },
-      currentId: '1',
-      currentProjectId: 'gitlab-org/gitlab-ce',
+      router,
     });
-  });
+  };
+  const findIcon = () => wrapper.find('.ic-mobile-issue-close');
 
   afterEach(() => {
-    vm.$destroy();
+    wrapper.destroy();
+    wrapper = null;
   });
 
-  it('renders merge requests data', () => {
-    expect(vm.$el.textContent).toContain('Merge request title');
-    expect(vm.$el.textContent).toContain('gitlab-org/gitlab-ce!1');
-  });
+  describe('default', () => {
+    beforeEach(() => {
+      createComponent();
+    });
 
-  it('renders link with href', () => {
-    const expectedHref = router.resolve(
-      `/project/${vm.item.projectPathWithNamespace}/merge_requests/${vm.item.iid}`,
-    ).href;
+    it('renders merge requests data', () => {
+      expect(wrapper.text()).toContain('Merge request title');
+      expect(wrapper.text()).toContain('gitlab-org/gitlab-ce!1');
+    });
 
-    expect(vm.$el.tagName.toLowerCase()).toBe('a');
-    expect(vm.$el).toHaveAttr('href', expectedHref);
-  });
+    it('renders link with href', () => {
+      const expectedHref = router.resolve(
+        `/project/${TEST_ITEM.projectPathWithNamespace}/merge_requests/${TEST_ITEM.iid}`,
+      ).href;
 
-  it('renders icon if ID matches currentId', () => {
-    expect(vm.$el.querySelector('.ic-mobile-issue-close')).not.toBe(null);
-  });
+      expect(wrapper.element.tagName.toLowerCase()).toBe('a');
+      expect(wrapper.attributes('href')).toBe(expectedHref);
+    });
 
-  it('does not render icon if ID does not match currentId', done => {
-    vm.currentId = '2';
+    it('renders icon if ID matches currentId', () => {
+      expect(findIcon().exists()).toBe(true);
+    });
+  });
 
-    vm.$nextTick(() => {
-      expect(vm.$el.querySelector('.ic-mobile-issue-close')).toBe(null);
+  describe('with different currentId', () => {
+    beforeEach(() => {
+      createComponent({ currentId: `${TEST_ITEM.iid + 1}` });
+    });
 
-      done();
+    it('does not render icon', () => {
+      expect(findIcon().exists()).toBe(false);
     });
   });
 
-  it('does not render icon if project ID does not match', done => {
-    vm.currentProjectId = 'test/test';
-
-    vm.$nextTick(() => {
-      expect(vm.$el.querySelector('.ic-mobile-issue-close')).toBe(null);
+  describe('with different project ID', () => {
+    beforeEach(() => {
+      createComponent({ currentProjectId: 'test/test' });
+    });
 
-      done();
+    it('does not render icon', () => {
+      expect(findIcon().exists()).toBe(false);
     });
   });
 });

spec/frontend/ide/stores/modules/router/actions_spec.js

+import * as actions from '~/ide/stores/modules/router/actions';
+import * as types from '~/ide/stores/modules/router/mutation_types';
+import testAction from 'helpers/vuex_action_helper';
+
+const TEST_PATH = 'test/path/abc';
+
+describe('ide/stores/modules/router/actions', () => {
+  describe('push', () => {
+    it('commits mutation', () => {
+      return testAction(
+        actions.push,
+        TEST_PATH,
+        {},
+        [{ type: types.PUSH, payload: TEST_PATH }],
+        [],
+      );
+    });
+  });
+});

spec/frontend/ide/stores/modules/router/mutations_spec.js

+import mutations from '~/ide/stores/modules/router/mutations';
+import * as types from '~/ide/stores/modules/router/mutation_types';
+import createState from '~/ide/stores/modules/router/state';
+
+const TEST_PATH = 'test/path/abc';
+
+describe('ide/stores/modules/router/mutations', () => {
+  let state;
+
+  beforeEach(() => {
+    state = createState();
+  });
+
+  describe(types.PUSH, () => {
+    it('updates state', () => {
+      expect(state.fullPath).toBe('');
+
+      mutations[types.PUSH](state, TEST_PATH);
+
+      expect(state.fullPath).toBe(TEST_PATH);
+    });
+  });
+});

spec/frontend/ide/sync_router_and_store_spec.js

+import Vue from 'vue';
+import VueRouter from 'vue-router';
+import Vuex from 'vuex';
+import routerModule from '~/ide/stores/modules/router';
+import { syncRouterAndStore } from '~/ide/sync_router_and_store';
+import waitForPromises from 'helpers/wait_for_promises';
+
+const TEST_ROUTE = '/test/lorem/ipsum';
+
+Vue.use(Vuex);
+
+describe('~/ide/sync_router_and_store', () => {
+  let unsync;
+  let router;
+  let store;
+  let onRouterChange;
+
+  const createSync = () => {
+    unsync = syncRouterAndStore(router, store);
+  };
+
+  const getRouterCurrentPath = () => router.currentRoute.fullPath;
+  const getStoreCurrentPath = () => store.state.router.fullPath;
+  const updateRouter = path => {
+    router.push(path);
+    return waitForPromises();
+  };
+  const updateStore = path => {
+    store.dispatch('router/push', path);
+    return waitForPromises();
+  };
+
+  beforeEach(() => {
+    router = new VueRouter();
+    store = new Vuex.Store({
+      modules: {
+        router: routerModule,
+      },
+    });
+    jest.spyOn(store, 'dispatch');
+
+    onRouterChange = jest.fn();
+    router.beforeEach((to, from, next) => {
+      onRouterChange(to, from);
+      next();
+    });
+  });
+
+  afterEach(() => {
+    unsync();
+    unsync = null;
+  });
+
+  it('keeps store and router in sync', async () => {
+    createSync();
+
+    await updateRouter('/test/test');
+    await updateRouter('/test/test');
+    await updateStore('123/abc');
+    await updateRouter('def');
+
+    // Even though we pused relative paths, the store and router kept track of the resulting fullPath
+    expect(getRouterCurrentPath()).toBe('/test/123/def');
+    expect(getStoreCurrentPath()).toBe('/test/123/def');
+  });
+
+  describe('default', () => {
+    beforeEach(() => {
+      createSync();
+    });
+
+    it('store is default', () => {
+      expect(store.dispatch).not.toHaveBeenCalled();
+      expect(getStoreCurrentPath()).toBe('');
+    });
+
+    it('router is default', () => {
+      expect(onRouterChange).not.toHaveBeenCalled();
+      expect(getRouterCurrentPath()).toBe('/');
+    });
+
+    describe('when store changes', () => {
+      beforeEach(() => {
+        updateStore(TEST_ROUTE);
+      });
+
+      it('store is updated', () => {
+        // let's make sure the action isn't dispatched more than necessary
+        expect(store.dispatch).toHaveBeenCalledTimes(1);
+        expect(getStoreCurrentPath()).toBe(TEST_ROUTE);
+      });
+
+      it('router is updated', () => {
+        expect(onRouterChange).toHaveBeenCalledTimes(1);
+        expect(getRouterCurrentPath()).toBe(TEST_ROUTE);
+      });
+
+      describe('when store changes again to the same thing', () => {
+        beforeEach(() => {
+          onRouterChange.mockClear();
+          updateStore(TEST_ROUTE);
+        });
+
+        it('doesnt change router again', () => {
+          expect(onRouterChange).not.toHaveBeenCalled();
+        });
+      });
+    });
+
+    describe('when router changes', () => {
+      beforeEach(() => {
+        updateRouter(TEST_ROUTE);
+      });
+
+      it('store is updated', () => {
+        expect(store.dispatch).toHaveBeenCalledTimes(1);
+        expect(getStoreCurrentPath()).toBe(TEST_ROUTE);
+      });
+
+      it('router is updated', () => {
+        // let's make sure the router change isn't triggered more than necessary
+        expect(onRouterChange).toHaveBeenCalledTimes(1);
+        expect(getRouterCurrentPath()).toBe(TEST_ROUTE);
+      });
+
+      describe('when router changes again to the same thing', () => {
+        beforeEach(() => {
+          store.dispatch.mockClear();
+          updateRouter(TEST_ROUTE);
+        });
+
+        it('doesnt change store again', () => {
+          expect(store.dispatch).not.toHaveBeenCalled();
+        });
+      });
+    });
+
+    describe('when disposed', () => {
+      beforeEach(() => {
+        unsync();
+      });
+
+      it('a store change does not trigger a router change', () => {
+        updateStore(TEST_ROUTE);
+
+        expect(getRouterCurrentPath()).toBe('/');
+        expect(onRouterChange).not.toHaveBeenCalled();
+      });
+
+      it('a router change does not trigger a store change', () => {
+        updateRouter(TEST_ROUTE);
+
+        expect(getStoreCurrentPath()).toBe('');
+        expect(store.dispatch).not.toHaveBeenCalled();
+      });
+    });
+  });
+});