-
Notifications
You must be signed in to change notification settings - Fork 27
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.
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
Conversation
b9b80b5
to
b5d5266
Compare
3b09bd2
to
d782fe6
Compare
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() | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Read the code - It seems sane, but I've been learning JS concepts on the fly.
- Checked it out and tried it locally - I was able to reproduce the bug on master and confirm that this branch fixes it. :)
Merging commit 8bfbfac (no changes)
These changes close #1248
Builds upon 4841f6a
When the view stays the same but the query changes (e.g. you navigate from
#/table/woof
to#/table/meow
), we need to unsubscribe and re-subscribe.Also, for a subscription to successfully start, you need to call, in order:
workflowService.subscribe(componentOrView)
workflowService.startSubscriptions()
Rather than split these between a mixin for component lifecycle hooks and a mixin for vue-router navigation guards respectively, I have combined them into the mixin for component lifecycle hooks.
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.