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.

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

UI: Polling Step 3 - Close connections when tabbing away #3953

Merged
merged 7 commits into from
Mar 8, 2018
14 changes: 13 additions & 1 deletion ui/app/components/client-node-row.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@ import { inject as service } from '@ember/service';
import Component from '@ember/component';
import { lazyClick } from '../helpers/lazy-click';
import { watchRelationship } from 'nomad-ui/utils/properties/watch';
import WithVisibilityDetection from 'nomad-ui/mixins/with-component-visibility-detection';

export default Component.extend({
export default Component.extend(WithVisibilityDetection, {
store: service(),

tagName: 'tr',
Expand All @@ -27,6 +28,17 @@ export default Component.extend({
}
},

visibilityHandler() {
if (document.hidden) {
this.get('watch').cancelAll();
} else {
const node = this.get('node');
if (node) {
this.get('watch').perform(node, 100);
}
}
},

willDestroy() {
this.get('watch').cancelAll();
this._super(...arguments);
Expand Down
14 changes: 13 additions & 1 deletion ui/app/components/job-row.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@ import { inject as service } from '@ember/service';
import Component from '@ember/component';
import { lazyClick } from '../helpers/lazy-click';
import { watchRelationship } from 'nomad-ui/utils/properties/watch';
import WithVisibilityDetection from 'nomad-ui/mixins/with-component-visibility-detection';

export default Component.extend({
export default Component.extend(WithVisibilityDetection, {
store: service(),

tagName: 'tr',
Expand All @@ -27,6 +28,17 @@ export default Component.extend({
}
},

visibilityHandler() {
if (document.hidden) {
this.get('watch').cancelAll();
} else {
const job = this.get('job');
if (job && !job.get('isLoading')) {
this.get('watch').perform(job, 100);
}
}
},

willDestroy() {
this.get('watch').cancelAll();
this._super(...arguments);
Expand Down
2 changes: 1 addition & 1 deletion ui/app/controllers/jobs/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export default Controller.extend(Sortable, Searchable, {
'system.namespaces.length',
function() {
const hasNamespaces = this.get('system.namespaces.length');
const activeNamespace = this.get('system.activeNamespace.id');
const activeNamespace = this.get('system.activeNamespace.id') || 'default';

return this.get('model')
.filter(job => !hasNamespaces || job.get('namespace.id') === activeNamespace)
Expand Down
12 changes: 12 additions & 0 deletions ui/app/mixins/with-component-visibility-detection.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import Mixin from '@ember/object/mixin';

export default Mixin.create({
setupDocumentVisibility: function() {
this.set('_visibilityHandler', this.get('visibilityHandler').bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

Will you ever have > 1 component with this mixin on a screen? Would you have a last one wins situation here?

Copy link

Choose a reason for hiding this comment

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

I think all components (or routes) would have their handlers run, right? Which is likely the desired behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes to both! Job row and node row both mix this mixin in, so multiple components. Yes all components own their own handler and independently add and remove their own handlers. This prevents accidental side effects when paginating or sorting resulting in one component being destroyed and bringing down all the visibility handlers.

document.addEventListener('visibilitychange', this.get('_visibilityHandler'));
}.on('init'),

removeDocumentVisibility: function() {
document.removeEventListener('visibilitychange', this.get('_visibilityHandler'));
}.on('willDestroy'),
});
12 changes: 12 additions & 0 deletions ui/app/mixins/with-route-visibility-detection.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import Mixin from '@ember/object/mixin';

export default Mixin.create({
setupDocumentVisibility: function() {
this.set('_visibilityHandler', this.get('visibilityHandler').bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I have the same question about nested routes as I do with multiple components - do outer routes have their handler overwritten by child routes here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, independent handlers again.

document.addEventListener('visibilitychange', this.get('_visibilityHandler'));
}.on('activate'),

removeDocumentVisibility: function() {
document.removeEventListener('visibilitychange', this.get('_visibilityHandler'));
}.on('deactivate'),
});
32 changes: 27 additions & 5 deletions ui/app/mixins/with-watchers.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,38 @@
import Mixin from '@ember/object/mixin';
import { computed } from '@ember/object';
import { assert } from '@ember/debug';
import WithVisibilityDetection from './with-route-visibility-detection';

export default Mixin.create({
export default Mixin.create(WithVisibilityDetection, {
Copy link
Contributor

Choose a reason for hiding this comment

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

mixin the mixin 🐢🐢🐢

watchers: computed(() => []),

cancelAllWatchers() {
this.get('watchers').forEach(watcher => {
assert('Watchers must be Ember Concurrency Tasks.', !!watcher.cancelAll);
watcher.cancelAll();
});
},

startWatchers() {
assert('startWatchers needs to be overridden in the Route', false);
Copy link
Contributor

Choose a reason for hiding this comment

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

❤ little usage helpers like this

},

setupController() {
this.startWatchers(...arguments);
return this._super(...arguments);
},

visibilityHandler() {
if (document.hidden) {
this.cancelAllWatchers();
} else {
this.startWatchers(this.controller, this.controller.get('model'));
}
},

actions: {
willTransition() {
this.get('watchers').forEach(watcher => {
assert('Watchers must be Ember Concurrency Tasks.', !!watcher.cancelAll);
watcher.cancelAll();
});
this.cancelAllWatchers();
},
},
});
3 changes: 1 addition & 2 deletions ui/app/routes/allocations/allocation.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@ import { watchRecord } from 'nomad-ui/utils/properties/watch';
import WithWatchers from 'nomad-ui/mixins/with-watchers';

export default Route.extend(WithModelErrorHandling, WithWatchers, {
setupController(controller, model) {
startWatchers(controller, model) {
controller.set('watcher', this.get('watch').perform(model));
return this._super(...arguments);
},

watch: watchRecord('allocation'),
Expand Down
5 changes: 4 additions & 1 deletion ui/app/routes/application.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { inject as service } from '@ember/service';
import Route from '@ember/routing/route';
import { AbortError } from 'ember-data/adapters/errors';

export default Route.extend({
config: service(),
Expand All @@ -22,7 +23,9 @@ export default Route.extend({
},

error(error) {
this.controllerFor('application').set('error', error);
if (!(error instanceof AbortError)) {
this.controllerFor('application').set('error', error);
}
},
},
});
3 changes: 1 addition & 2 deletions ui/app/routes/clients/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,9 @@ export default Route.extend(WithWatchers, {
return model && model.get('allocations');
},

setupController(controller, model) {
startWatchers(controller, model) {
controller.set('watchModel', this.get('watch').perform(model));
controller.set('watchAllocations', this.get('watchAllocations').perform(model));
return this._super(...arguments);
},

watch: watchRecord('node'),
Expand Down
3 changes: 1 addition & 2 deletions ui/app/routes/clients/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@ import { watchAll } from 'nomad-ui/utils/properties/watch';
import WithWatchers from 'nomad-ui/mixins/with-watchers';

export default Route.extend(WithWatchers, {
setupController(controller) {
startWatchers(controller) {
controller.set('watcher', this.get('watch').perform());
return this._super(...arguments);
},

watch: watchAll('node'),
Expand Down
3 changes: 1 addition & 2 deletions ui/app/routes/jobs/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@ import { watchAll } from 'nomad-ui/utils/properties/watch';
import WithWatchers from 'nomad-ui/mixins/with-watchers';

export default Route.extend(WithWatchers, {
setupController(controller) {
startWatchers(controller) {
controller.set('modelWatch', this.get('watch').perform());
return this._super(...arguments);
},

watch: watchAll('job'),
Expand Down
3 changes: 1 addition & 2 deletions ui/app/routes/jobs/job/deployments.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,9 @@ export default Route.extend(WithWatchers, {
return RSVP.all([job.get('deployments'), job.get('versions')]).then(() => job);
},

setupController(controller, model) {
startWatchers(controller, model) {
controller.set('watchDeployments', this.get('watchDeployments').perform(model));
controller.set('watchVersions', this.get('watchVersions').perform(model));
return this._super(...arguments);
},

watchDeployments: watchRelationship('deployments'),
Expand Down
4 changes: 1 addition & 3 deletions ui/app/routes/jobs/job/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,13 @@ import { watchRecord, watchRelationship } from 'nomad-ui/utils/properties/watch'
import WithWatchers from 'nomad-ui/mixins/with-watchers';

export default Route.extend(WithWatchers, {
setupController(controller, model) {
startWatchers(controller, model) {
controller.set('watchers', {
model: this.get('watch').perform(model),
summary: this.get('watchSummary').perform(model),
evaluations: this.get('watchEvaluations').perform(model),
deployments: this.get('watchDeployments').perform(model),
});

return this._super(...arguments);
},

watch: watchRecord('job'),
Expand Down
3 changes: 1 addition & 2 deletions ui/app/routes/jobs/job/task-group.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,13 @@ export default Route.extend(WithWatchers, {
});
},

setupController(controller, model) {
startWatchers(controller, model) {
const job = model.get('job');
controller.set('watchers', {
job: this.get('watchJob').perform(job),
summary: this.get('watchSummary').perform(job),
allocations: this.get('watchAllocations').perform(job),
});
return this._super(...arguments);
},

watchJob: watchRecord('job'),
Expand Down
3 changes: 1 addition & 2 deletions ui/app/routes/jobs/job/versions.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,8 @@ export default Route.extend(WithWatchers, {
return job.get('versions').then(() => job);
},

setupController(controller, model) {
startWatchers(controller, model) {
controller.set('watcher', this.get('watchVersions').perform(model));
return this._super(...arguments);
},

watchVersions: watchRelationship('versions'),
Expand Down
1 change: 1 addition & 0 deletions ui/mirage/factories/job.js
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ export default Factory.extend({
const jobSummary = server.create('job-summary', hasChildren ? 'withChildren' : 'withSummary', {
groupNames: groups.mapBy('name'),
job,
job_id: job.id,
namespace: job.namespace,
});

Expand Down