Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug where subscriptions would not update on query change #1249

Merged
merged 4 commits into from
Apr 21, 2023
Merged

Fix bug where subscriptions would not update on query change #1249

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f9d64eb
Fix bug where subscriptions would not update on query change
MetRonnie Feb 21, 2023
d782fe6
Update changelog
MetRonnie Feb 22, 2023
ce869a7
Ensure lumino tabs are created with unique IDs
MetRonnie Mar 3, 2023
0440c23
Merge branch 'master' into subscriptions
MetRonnie Mar 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@ creating a new release entry be sure to copy & paste the span tag with the
`actions:bind` attribute, which is used by a regex to find the text to be
updated. Only the first match gets replaced, so it's fine to leave the old
ones in. -->
-------------------------------------------------------------------------------
## __cylc-ui-1.6.0 (<span actions:bind='release-date'>Upcoming</span>)__

### Fixes

[#1249](https://github.com/cylc/cylc-ui/pull/1249) - Fix tasks not loading
when navigating between workflows in the standalone `#/tree/` and `#/table/`
views.

-------------------------------------------------------------------------------
## __cylc-ui-1.5.0 (<span actions:bind='release-date'>Released 2023-02-20</span>)__

Expand Down
9 changes: 9 additions & 0 deletions src/mixins/subscriptionComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,17 @@ export default {
],
beforeMount () {
this.$workflowService.subscribe(this)
this.$workflowService.startSubscriptions()
},
beforeDestroy () {
this.$workflowService.unsubscribe(this)
},
watch: {
query () {
// if the query changes, unsubscribe & re-subscribe
this.$workflowService.unsubscribe(this)
this.$workflowService.subscribe(this)
this.$workflowService.startSubscriptions()
}
Comment on lines 36 to +49
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah heck, something I hadn't thought about is what happens when there are multiple "views" in the workspace.

Currently we always initiate a workspace with a single view (because we don't remember the previous tab layout and haven't got default layouts sorted yet). But soon we will be initiating views with multiple components.

I think that calling startSubscriptions in either beforeMount or watch.query would result in subscribing and unsubscribing multiple times as each view registers its subscription in turn. This is probably why this call was bound to navigation events rather than component lifecycle events (as we did briefly remember tab layout by accident in an earlier version).

So we want to delay running $workflowService.startSubscriptions() until $workflowService.subscribe(this) has run for all views, not just the first. I'm not sure how to do this without introducing an arbitrary timeout.

Perhaps the parent view should be responsible for registering subscriptions before the child component is mounted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at

cylc-ui/src/services/workflow.service.js

Lines 241 to 250 in b9fe3a8

/**
* Start any pending subscriptions.
*/
startSubscriptions () {
const pendingSubscriptions = Object.values(this.subscriptions)
.filter(subscription => {
return subscription.observable === null || subscription.reload
})
pendingSubscriptions.forEach(subscription => this.startSubscription(subscription))
}

There is no difference between calling both $workflowService.subscribe(vm) and $workflowService.startSubscriptions() multiple times vs calling the former multiple times and the latter only once.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly that means the problem is larger than anticipated. We will need some mechanism for delaying the subscription start until all of the components have registered their sub-subscriptions to avoid creating/cancelling subs multiple times on startup.

Copy link
Member Author

@MetRonnie MetRonnie Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ok to go in, with a follow-up issue for dealing with multiple subscriptions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1248 isn't really a problem so there's no real pressure to get this in?

Copy link
Member Author

@MetRonnie MetRonnie Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aye but in the Vue 3 upgrade this also fixes the critical problem of tasks not loading when navigating between different workflows in the Workspace view

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did this discussion end up?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is currently being used within the Vue 3 work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it would be sensible to merge this before Vue 3 if we are happy it works. vuejs/router#1710:

Routing Lifecycle and Component Lifecycle are independent. Callbacks passed to hooks should therefore also be independent and not rely on the order.

Merging this now would take it out of the already-large diff of the Vue 3 PR.

I suggest we open a separate issue to document Oliver's point

We will need some mechanism for delaying the subscription start until all of the components have registered their sub-subscriptions to avoid creating/cancelling subs multiple times on startup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1278

}
}
40 changes: 0 additions & 40 deletions src/mixins/subscriptionView.js
View file
Open in desktop

This file was deleted.

4 changes: 1 addition & 3 deletions src/views/Dashboard.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,6 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.
import { mapState, mapGetters } from 'vuex'
import { mdiBook, mdiBookMultiple, mdiBookOpenVariant, mdiCog, mdiHubspot, mdiTable } from '@mdi/js'
import pageMixin from '@/mixins/index'
import subscriptionViewMixin from '@/mixins/subscriptionView'
import subscriptionComponentMixin from '@/mixins/subscriptionComponent'
import { createUrl } from '@/utils/urls'
import { WorkflowState, WorkflowStateOrder } from '@/model/WorkflowState.model'
Expand All @@ -179,8 +178,7 @@ export default {
name: 'Dashboard',
mixins: [
pageMixin,
subscriptionComponentMixin,
subscriptionViewMixin
subscriptionComponentMixin
],
metaInfo () {
return {
Expand Down
4 changes: 1 addition & 3 deletions src/views/Graph.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ import gql from 'graphql-tag'
import { mapGetters } from 'vuex'
import pageMixin from '@/mixins/index'
import graphqlMixin from '@/mixins/graphql'
import subscriptionViewMixin from '@/mixins/subscriptionView'
import subscriptionComponentMixin from '@/mixins/subscriptionComponent'
import SubscriptionQuery from '@/model/SubscriptionQuery.model'
// import CylcTreeCallback from '@/services/treeCallback'
Expand Down Expand Up @@ -203,8 +202,7 @@ export default {
mixins: [
pageMixin,
graphqlMixin,
subscriptionComponentMixin,
subscriptionViewMixin
subscriptionComponentMixin
],
name: 'Graph',
components: {
Expand Down
2 changes: 0 additions & 2 deletions src/views/Guide.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,11 +191,9 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.
import Task from '@/components/cylc/Task'
import Job from '@/components/cylc/Job'
import { TaskStateUserOrder } from '@/model/TaskState.model'
import subscriptionViewMixin from '@/mixins/subscriptionView'

export default {
name: 'Guide',
mixins: [subscriptionViewMixin],
components: {
task: Task,
job: Job
Expand Down
6 changes: 2 additions & 4 deletions src/views/Log.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,7 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.
import { mdiFileDocumentMultipleOutline } from '@mdi/js'
import pageMixin from '@/mixins/index'
import graphqlMixin from '@/mixins/graphql'
import subscriptionViewMixin from '@/mixins/subscriptionView'
import subscriptionMixin from '@/mixins/subscriptionComponent'
import subscriptionComponentMixin from '@/mixins/subscriptionComponent'
import LogComponent from '@/components/cylc/log/Log'
import SubscriptionQuery from '@/model/SubscriptionQuery.model'
import { LOGS_SUBSCRIPTION } from '@/graphql/queries'
Expand All @@ -116,8 +115,7 @@ export default {
mixins: [
pageMixin,
graphqlMixin,
subscriptionMixin,
subscriptionViewMixin
subscriptionComponentMixin
],
name: 'Log',
components: {
Expand Down
4 changes: 1 addition & 3 deletions src/views/Table.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import { mapState, mapGetters } from 'vuex'
import { mdiTable } from '@mdi/js'
import pageMixin from '@/mixins/index'
import graphqlMixin from '@/mixins/graphql'
import subscriptionViewMixin from '@/mixins/subscriptionView'
import subscriptionComponentMixin from '@/mixins/subscriptionComponent'
import TableComponent from '@/components/cylc/table/Table.vue'
import SubscriptionQuery from '@/model/SubscriptionQuery.model'
Expand All @@ -43,8 +42,7 @@ export default {
mixins: [
pageMixin,
graphqlMixin,
subscriptionComponentMixin,
subscriptionViewMixin
subscriptionComponentMixin
],
name: 'Table',
components: {
Expand Down
4 changes: 1 addition & 3 deletions src/views/Tree.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import { mapState, mapGetters } from 'vuex'
import { mdiFileTree } from '@mdi/js'
import pageMixin from '@/mixins/index'
import graphqlMixin from '@/mixins/graphql'
import subscriptionViewMixin from '@/mixins/subscriptionView'
import subscriptionComponentMixin from '@/mixins/subscriptionComponent'
import SubscriptionQuery from '@/model/SubscriptionQuery.model'
import TreeComponent from '@/components/cylc/tree/Tree'
Expand All @@ -48,8 +47,7 @@ export default {
mixins: [
pageMixin,
graphqlMixin,
subscriptionComponentMixin,
subscriptionViewMixin
subscriptionComponentMixin
],
name: 'Tree',
components: {
Expand Down
4 changes: 1 addition & 3 deletions src/views/UserProfile.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,15 +215,13 @@ import { decreaseFontSize, getCurrentFontSize, increaseFontSize, resetFontSize }
import { mdiCog, mdiFormatFontSizeDecrease, mdiFormatFontSizeIncrease } from '@mdi/js'
import Job from '@/components/cylc/Job'
import JobState from '@/model/JobState.model'
import subscriptionViewMixin from '@/mixins/subscriptionView'

// TODO: update where user preferences are stored after #335

export default {
name: 'UserProfile',
mixins: [
pageMixin,
subscriptionViewMixin
pageMixin
],
components: {
Job
Expand Down
4 changes: 1 addition & 3 deletions src/views/Workflows.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,14 @@ import GScan from '@/components/cylc/gscan/GScan'
import { GSCAN_DELTAS_SUBSCRIPTION } from '@/graphql/queries'
import pageMixin from '@/mixins/index'
import subscriptionComponentMixin from '@/mixins/subscriptionComponent'
import subscriptionViewMixin from '@/mixins/subscriptionView'
import SubscriptionQuery from '@/model/SubscriptionQuery.model'

export default {
name: 'Workflows',

mixins: [
pageMixin,
subscriptionComponentMixin,
subscriptionViewMixin
subscriptionComponentMixin
],

components: {
Expand Down
4 changes: 2 additions & 2 deletions src/views/WorkflowsTable.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ import { mapState, mapGetters } from 'vuex'
import i18n from '@/i18n'
import { mdiTable } from '@mdi/js'
import pageMixin from '@/mixins/index'
import subscriptionViewMixin from '@/mixins/subscriptionView'
import subscriptionMixin from '@/mixins/subscription'
import SubscriptionQuery from '@/model/SubscriptionQuery.model'
import { WORKFLOWS_TABLE_DELTAS_SUBSCRIPTION } from '@/graphql/queries'
import WorkflowIcon from '@/components/cylc/gscan/WorkflowIcon'
Expand All @@ -81,7 +81,7 @@ export default {
name: 'WorkflowsTable',
mixins: [
pageMixin,
subscriptionViewMixin
subscriptionMixin
],
metaInfo () {
return {
Expand Down
11 changes: 2 additions & 9 deletions src/views/Workspace.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ import Vue from 'vue'
import { each, iter } from '@lumino/algorithm'
import pageMixin from '@/mixins'
import graphqlMixin from '@/mixins/graphql'
import subscriptionViewMixin from '@/mixins/subscriptionView'
import subscriptionMixin from '@/mixins/subscription'
import ViewState from '@/model/ViewState.model'
import { createWidgetId } from '@/components/cylc/workflow/index'
import Lumino from '@/components/cylc/workflow/Lumino'
Expand All @@ -70,7 +70,7 @@ export default {
mixins: [
pageMixin,
graphqlMixin,
subscriptionViewMixin
subscriptionMixin
],

components: {
Expand Down Expand Up @@ -179,13 +179,6 @@ export default {
createWidgetId(),
{ view, initialOptions }
)
this.$nextTick(() => {
// Views use navigation-guards to start the pending subscriptions. But we don't have
// this in this view. We must pretend we are doing the beforeRouteEnter here, and
// call the .startSubscriptions function, so that the WorkflowService will take care
// of loading the pending subscriptions (like the ones created by the new view).
this.$workflowService.startSubscriptions()
})
},
/**
* Remove all the widgets present in the UI.
Expand Down
3 changes: 2 additions & 1 deletion tests/unit/components/cylc/drawer.vue.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

import { expect } from 'chai'
import { createLocalVue, mount } from '@vue/test-utils'
import Drawer from '../../../../src/components/cylc/Drawer'
import Drawer from '@/components/cylc/Drawer'

import Vue from 'vue'
import Vuetify from 'vuetify'
Expand All @@ -37,6 +37,7 @@ const mountFunction = options => {
register () {},
unregister () {},
subscribe () {},
startSubscriptions () {},
introspection: Promise.resolve({
mutations: [
{ args: [] }
Expand Down
47 changes: 25 additions & 22 deletions tests/unit/mixins/subscriptionComponent.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,46 +16,49 @@
*/

import { expect } from 'chai'
import sinon from 'sinon'
import { createLocalVue, shallowMount } from '@vue/test-utils'
import subscriptionComponentMixin from '@/mixins/subscriptionComponent'
import WorkflowService from '@/services/workflow.service'

const localVue = createLocalVue()

describe('Subscription Component mixin', () => {
let workflowService
let workflowService, Component
beforeEach(() => {
workflowService = {
subscribe (componentOrView) {
componentOrView.subscribed = true
},
unsubscribe (componentOrView) {
componentOrView.subscribed = false
}
}
workflowService = sinon.createStubInstance(WorkflowService)
localVue.prototype.$workflowService = workflowService
})
it('should provide a hook for when the component is created', () => {
const Component = {

Component = {
mixins: [subscriptionComponentMixin],
data: () => ({
query: { foo: 1 }
}),
render () {
}
}
})

it('subscribes & unsubscribes when the component is mounted & destroyed', () => {
const component = shallowMount(Component, {
localVue
})
expect(component.vm.subscribed).to.equal(true)
expect(workflowService.subscribe.calledOnceWith(component.vm)).to.equal(true)
expect(workflowService.startSubscriptions.calledOnce).to.equal(true)
expect(workflowService.unsubscribe.called).to.equal(false)
component.vm.$destroy()
expect(workflowService.unsubscribe.calledOnceWith(component.vm)).to.equal(true)
})
it('should provide a hook for when the component is destroyed', () => {
const Component = {
mixins: [subscriptionComponentMixin],
render () {
}
}

it('un- & re-subcribes when the query changes', () => {
const component = shallowMount(Component, {
localVue
})
expect(component.vm.subscribed).to.equal(true)
component.vm.$destroy()
expect(component.vm.subscribed).to.equal(false)
component.vm.query = { foo: 2 }
component.vm.$nextTick(() => {
expect(workflowService.unsubscribe.calledOnceWith(component.vm)).to.equal(true)
expect(workflowService.subscribe.calledTwice).to.equal(true)
expect(workflowService.startSubscriptions.calledTwice).to.equal(true)
})
})
})
44 changes: 0 additions & 44 deletions tests/unit/mixins/subscriptionView.spec.js
View file
Open in desktop

This file was deleted.