WorkflowService: Fix TypeError when unsubscribing

This affected the log view when switching between workflow & job logs.

Author: MetRonnie

src/mixins/subscriptionComponent.js

@@ -49,11 +49,11 @@ export default {
     this._updateQuery(null, this.query)
   },
   methods: {
-    _updateQuery (newVal, oldVal) {
-      if (oldVal) {
-        this.$workflowService.unsubscribe(this)
+    _updateQuery (newQuery, oldQuery) {
+      if (oldQuery) {
+        this.$workflowService.unsubscribe(oldQuery, this._uid)
       }
-      if (newVal) {
+      if (newQuery) {
         this.$workflowService.subscribe(this)
         this.$workflowService.startSubscriptions()
       }

src/services/workflow.service.js

@@ -194,25 +194,20 @@ class WorkflowService {
   // --- GraphQL query subscriptions
 
   /**
-   * @param {View} componentOrView
+   * @param {SubscriptionQuery} query
    * @returns {Subscription}
    */
-  getOrCreateSubscription (componentOrView) {
-    const queryName = componentOrView.query.name
-    let subscription = this.subscriptions[queryName]
+  getOrCreateSubscription (query) {
     // note, this will force a return of the FIRST query of the SAME name as any subsequent queries
-    if (!subscription) {
-      subscription = this.subscriptions[queryName] = new Subscription(componentOrView.query)
-    }
-    return subscription
+    return (this.subscriptions[query.name] ??= new Subscription(query))
   }
 
   /**
    * @param {View} componentOrView
    */
   subscribe (componentOrView) {
     // First we retrieve the existing, or create a new subscription (and add to the pool).
-    const subscription = this.getOrCreateSubscription(componentOrView)
+    const subscription = this.getOrCreateSubscription(componentOrView.query)
     if (!subscription.subscribers[componentOrView._uid]) {
       // NOTE: make sure to remove it afterwards to avoid memory leaks!
       subscription.subscribers[componentOrView._uid] = componentOrView
@@ -398,15 +393,21 @@ class WorkflowService {
     })
   }
 
-  unsubscribe (componentOrView) {
-    const subscription = this.subscriptions[componentOrView.query.name]
+  /**
+   * Remove subscriber and stop subscription.
+   *
+   * @param {SubscriptionQuery} query - The component/view's subscription query.
+   * @param {string} uid - The unique ID for the component/view.
+   */
+  unsubscribe (query, uid) {
+    const subscription = this.subscriptions[query.name]
     if (!subscription) {
       // eslint-disable-next-line no-console
-      console.warn(`Could not unsubscribe [${componentOrView.query.name}]: Not Found`)
+      console.warn(`Could not unsubscribe [${query.name}]: Not Found`)
       return
     }
     // Remove viewOrComponent subscriber
-    delete subscription.subscribers[componentOrView._uid]
+    delete subscription.subscribers[uid]
     // If no more subscribers, then stop the subscription.
     if (Object.keys(subscription.subscribers).length === 0) {
       this.stopSubscription(subscription)

tests/unit/mixins/subscriptionComponent.spec.js

@@ -19,6 +19,7 @@ import sinon from 'sinon'
 import { mount } from '@vue/test-utils'
 import subscriptionComponentMixin from '@/mixins/subscriptionComponent'
 import WorkflowService from '@/services/workflow.service'
+import { cloneDeep } from 'lodash'
 
 describe('Subscription Component mixin', () => {
   let $workflowService, component
@@ -40,17 +41,20 @@ describe('Subscription Component mixin', () => {
   })
 
   it('subscribes & unsubscribes when the component is mounted & destroyed', () => {
-    expect($workflowService.subscribe.calledOnceWith(component.vm)).to.equal(true)
+    const { vm } = component
+    expect($workflowService.subscribe.calledOnceWith(vm)).to.equal(true)
     expect($workflowService.startSubscriptions.calledOnce).to.equal(true)
     expect($workflowService.unsubscribe.called).to.equal(false)
     component.unmount()
-    expect($workflowService.unsubscribe.calledOnceWith(component.vm)).to.equal(true)
+    expect($workflowService.unsubscribe.calledOnceWith(vm.query, vm._uid)).to.equal(true)
   })
 
   it('un- & re-subcribes when the query changes', () => {
-    component.vm.query = { foo: 2 }
-    component.vm.$nextTick(() => {
-      expect($workflowService.unsubscribe.calledOnceWith(component.vm)).to.equal(true)
+    const { vm } = component
+    const oldQuery = cloneDeep(vm.query)
+    vm.query = { foo: 2 }
+    vm.$nextTick(() => {
+      expect($workflowService.unsubscribe.calledOnceWith(oldQuery, vm._uid)).to.equal(true)
       expect($workflowService.subscribe.calledTwice).to.equal(true)
       expect($workflowService.startSubscriptions.calledTwice).to.equal(true)
     })

tests/unit/services/workflow.service.spec.js

@@ -130,13 +130,13 @@ describe('WorkflowService', () => {
   })
   describe('getOrCreateSubscription', () => {
     it('should return existing subscriptions', () => {
-      const existingSubscription = service.getOrCreateSubscription(view)
+      const existingSubscription = service.getOrCreateSubscription(view.query)
       expect(existingSubscription).to.deep.equal(subscription)
     })
     it('should create new subscriptions and add to local cache', () => {
       delete service.subscriptions[view.query.name]
       expect(Object.keys(service.subscriptions).length).to.equal(0)
-      const newSubscription = service.getOrCreateSubscription(view)
+      const newSubscription = service.getOrCreateSubscription(view.query)
       expect(Object.keys(service.subscriptions).length).to.equal(1)
       expect(service.subscriptions[view.query.name]).to.deep.equal(newSubscription)
     })
@@ -343,16 +343,12 @@ describe('WorkflowService', () => {
   describe('unsubscribe', () => {
     it('should warn about queries that do not exist', () => {
       const stub = sandbox.stub(console, 'warn')
-      service.unsubscribe({
-        query: {
-          name: 'missing'
-        }
-      })
+      service.unsubscribe({ name: 'missing' }, 'irrelevant_uid')
       expect(stub.calledOnce).to.equal(true)
     })
     it('should call unsubscribe if last subscriber is unsubscribed', () => {
       const stub = sandbox.stub(service, 'stopSubscription')
-      service.unsubscribe(view)
+      service.unsubscribe(view.query, view._uid)
       expect(stub.calledOnce).to.equal(true)
     })
     it('should NOT call unsubscribe if there are still subscribers left', () => {
@@ -362,7 +358,7 @@ describe('WorkflowService', () => {
       }
       service.subscribe(anotherView)
       const stub = sandbox.stub(service, 'stopSubscription')
-      service.unsubscribe(view)
+      service.unsubscribe(view.query, view._uid)
       expect(stub.calledOnce).to.equal(false)
     })
   })