From 52705125a14c3053ac46067151072da4a99e7741 Mon Sep 17 00:00:00 2001 From: John Cowen Date: Wed, 17 Jun 2020 14:19:50 +0100 Subject: [PATCH] ui: Remove WithEventSource mixin, use a component instead (#7953) The WithEventSource mixin was responsible for catching EventSource errors and cleaning up events sources then the user left a Controller. As we are trying to avoid mixin usage, we moved this all to an `EventSource` component, which can clean up when the component is removed from the page, and also fires an onerror event. Moving to a component firing an onerror event means we can also remove all of our custom computed property work that we were using previously to catch errors (thrown when a service etc. is removed) --- ui-v2/app/components/event-source/README.mdx | 25 ++++++++ ui-v2/app/components/event-source/index.js | 38 ++++++++++++ .../app/components/tabular-details/index.hbs | 2 + ui-v2/app/components/tabular-details/index.js | 12 +--- ui-v2/app/computed/catchable.js | 11 ---- ui-v2/app/controllers/dc/intentions/index.js | 3 +- ui-v2/app/controllers/dc/nodes/index.js | 3 +- ui-v2/app/controllers/dc/nodes/show.js | 36 ++++++----- ui-v2/app/controllers/dc/nspaces/index.js | 3 +- ui-v2/app/controllers/dc/services/index.js | 4 +- ui-v2/app/controllers/dc/services/instance.js | 36 +++++------ ui-v2/app/controllers/dc/services/show.js | 36 ++++++----- .../app/instance-initializers/event-source.js | 2 +- ui-v2/app/mixins/with-event-source.js | 61 ------------------- ui-v2/app/templates/dc/intentions/index.hbs | 1 + ui-v2/app/templates/dc/nodes/index.hbs | 1 + ui-v2/app/templates/dc/nodes/show.hbs | 1 + ui-v2/app/templates/dc/nspaces/index.hbs | 1 + ui-v2/app/templates/dc/services/index.hbs | 1 + ui-v2/app/templates/dc/services/instance.hbs | 2 + ui-v2/app/templates/dc/services/show.hbs | 3 + ui-v2/app/utils/computed/purify.js | 42 ------------- ui-v2/app/utils/dom/event-source/proxy.js | 20 ++---- .../components/event-source-test.js | 26 ++++++++ 24 files changed, 175 insertions(+), 195 deletions(-) create mode 100644 ui-v2/app/components/event-source/README.mdx create mode 100644 ui-v2/app/components/event-source/index.js delete mode 100644 ui-v2/app/computed/catchable.js delete mode 100644 ui-v2/app/mixins/with-event-source.js delete mode 100644 ui-v2/app/utils/computed/purify.js create mode 100644 ui-v2/tests/integration/components/event-source-test.js diff --git a/ui-v2/app/components/event-source/README.mdx b/ui-v2/app/components/event-source/README.mdx new file mode 100644 index 000000000..589176cc8 --- /dev/null +++ b/ui-v2/app/components/event-source/README.mdx @@ -0,0 +1,25 @@ +## EventSource + +```handlebars + +``` + +### Arguments + +| Argument | Type | Default | Description | +| --- | --- | --- | --- | +| `src` | `EventSourceProxy` | | An EventSource object | +| `onerror` | `Function` | | The action to fire when an error occurs. Emits ErrorEvent object with an `error` property containing the Error. | +| `closeOnDestroy` | `Boolean` | true | Whether to close and destroy the event source when the component is destroyed | + +This component is used to configure event source error from within a template, but also ensures that EventSources are cleaned/up destroyed when the user leaves the page (when the component is removed from the page) + +### See + +- [Component Source Code](./index.js) +- [Template Source Code](./index.hbs) + +--- diff --git a/ui-v2/app/components/event-source/index.js b/ui-v2/app/components/event-source/index.js new file mode 100644 index 000000000..c0f1fc573 --- /dev/null +++ b/ui-v2/app/components/event-source/index.js @@ -0,0 +1,38 @@ +import Component from '@ember/component'; +import { inject as service } from '@ember/service'; + +export default Component.extend({ + tagName: '', + dom: service('dom'), + logger: service('logger'), + closeOnDestroy: true, + onerror: function(e) { + this.logger.execute(e.error); + }, + init: function() { + this._super(...arguments); + this._listeners = this.dom.listeners(); + }, + willDestroyElement: function() { + if (this.closeOnDestroy && typeof (this.src || {}).close === 'function') { + this.src.close(); + this.src.willDestroy(); + } + this._listeners.remove(); + this._super(...arguments); + }, + didReceiveAttrs: function() { + this._listeners.remove(); + if (typeof (this.src || {}).addEventListener === 'function') { + this._listeners.add(this.src, { + error: e => { + try { + this.onerror(e); + } catch (err) { + this.logger.execute(e.error); + } + }, + }); + } + }, +}); diff --git a/ui-v2/app/components/tabular-details/index.hbs b/ui-v2/app/components/tabular-details/index.hbs index 90aabf96b..dfa52cdd7 100644 --- a/ui-v2/app/components/tabular-details/index.hbs +++ b/ui-v2/app/components/tabular-details/index.hbs @@ -7,6 +7,7 @@ + {{#let (concat 'tabular-details-' name '-toggle-' guid '_') as |inputId|}} {{#each items as |item index|}} {{yield item index}} @@ -28,5 +29,6 @@ {{/each}} + {{/let}} diff --git a/ui-v2/app/components/tabular-details/index.js b/ui-v2/app/components/tabular-details/index.js index 8ce0a5eae..91f758d0d 100644 --- a/ui-v2/app/components/tabular-details/index.js +++ b/ui-v2/app/components/tabular-details/index.js @@ -1,20 +1,14 @@ import Component from '@ember/component'; -import SlotsMixin from 'block-slots'; import { inject as service } from '@ember/service'; -import { set } from '@ember/object'; -import { subscribe } from 'consul-ui/utils/computed/purify'; +import Slotted from 'block-slots'; -let uid = 0; -export default Component.extend(SlotsMixin, { +export default Component.extend(Slotted, { dom: service('dom'), onchange: function() {}, init: function() { this._super(...arguments); - set(this, 'uid', uid++); + this.guid = this.dom.guid(this); }, - inputId: subscribe('name', 'uid', function(name = 'name') { - return `tabular-details-${name}-toggle-${uid}_`; - }), actions: { click: function(e) { this.dom.clickFirstAnchor(e); diff --git a/ui-v2/app/computed/catchable.js b/ui-v2/app/computed/catchable.js deleted file mode 100644 index 29ab7a41b..000000000 --- a/ui-v2/app/computed/catchable.js +++ /dev/null @@ -1,11 +0,0 @@ -import { computed as computedPropertyFactory } from '@ember/object'; - -export const computed = function() { - const prop = computedPropertyFactory(...arguments); - prop.catch = function(cb) { - return this.meta({ - catch: cb, - }); - }; - return prop; -}; diff --git a/ui-v2/app/controllers/dc/intentions/index.js b/ui-v2/app/controllers/dc/intentions/index.js index b3ddea57f..c596e5150 100644 --- a/ui-v2/app/controllers/dc/intentions/index.js +++ b/ui-v2/app/controllers/dc/intentions/index.js @@ -1,6 +1,5 @@ import Controller from '@ember/controller'; -import WithEventSource from 'consul-ui/mixins/with-event-source'; -export default Controller.extend(WithEventSource, { +export default Controller.extend({ queryParams: { filterBy: { as: 'action', diff --git a/ui-v2/app/controllers/dc/nodes/index.js b/ui-v2/app/controllers/dc/nodes/index.js index ec975f903..5454ee88a 100644 --- a/ui-v2/app/controllers/dc/nodes/index.js +++ b/ui-v2/app/controllers/dc/nodes/index.js @@ -1,7 +1,6 @@ import Controller from '@ember/controller'; -import WithEventSource from 'consul-ui/mixins/with-event-source'; -export default Controller.extend(WithEventSource, { +export default Controller.extend({ queryParams: { filterBy: { as: 'status', diff --git a/ui-v2/app/controllers/dc/nodes/show.js b/ui-v2/app/controllers/dc/nodes/show.js index 0c23f88a2..98b8755b4 100644 --- a/ui-v2/app/controllers/dc/nodes/show.js +++ b/ui-v2/app/controllers/dc/nodes/show.js @@ -2,25 +2,29 @@ import Controller from '@ember/controller'; import { inject as service } from '@ember/service'; import { get } from '@ember/object'; import { alias } from '@ember/object/computed'; -import WithEventSource, { listen } from 'consul-ui/mixins/with-event-source'; -export default Controller.extend(WithEventSource, { +export default Controller.extend({ dom: service('dom'), notify: service('flashMessages'), items: alias('item.Services'), - item: listen('item').catch(function(e) { - if (e.target.readyState === 1) { - // OPEN - if (get(e, 'error.errors.firstObject.status') === '404') { - this.notify.add({ - destroyOnClick: false, - sticky: true, - type: 'warning', - action: 'update', - }); - this.tomography.close(); - this.sessions.close(); + actions: { + error: function(e) { + if (e.target.readyState === 1) { + // OPEN + if (get(e, 'error.errors.firstObject.status') === '404') { + this.notify.add({ + destroyOnClick: false, + sticky: true, + type: 'warning', + action: 'update', + }); + [e.target, this.tomography, this.sessions].forEach(function(item) { + if (item && typeof item.close === 'function') { + item.close(); + } + }); + } } - } - }), + }, + }, }); diff --git a/ui-v2/app/controllers/dc/nspaces/index.js b/ui-v2/app/controllers/dc/nspaces/index.js index d7569cb2d..0584aee0a 100644 --- a/ui-v2/app/controllers/dc/nspaces/index.js +++ b/ui-v2/app/controllers/dc/nspaces/index.js @@ -1,7 +1,6 @@ import Controller from '@ember/controller'; -import WithEventSource from 'consul-ui/mixins/with-event-source'; -export default Controller.extend(WithEventSource, { +export default Controller.extend({ queryParams: { search: { as: 'filter', diff --git a/ui-v2/app/controllers/dc/services/index.js b/ui-v2/app/controllers/dc/services/index.js index 251319246..2847f962e 100644 --- a/ui-v2/app/controllers/dc/services/index.js +++ b/ui-v2/app/controllers/dc/services/index.js @@ -1,7 +1,7 @@ import Controller from '@ember/controller'; import { computed } from '@ember/object'; -import WithEventSource from 'consul-ui/mixins/with-event-source'; -export default Controller.extend(WithEventSource, { + +export default Controller.extend({ queryParams: { sortBy: 'sort', search: { diff --git a/ui-v2/app/controllers/dc/services/instance.js b/ui-v2/app/controllers/dc/services/instance.js index 072478d0f..f98b6c0fc 100644 --- a/ui-v2/app/controllers/dc/services/instance.js +++ b/ui-v2/app/controllers/dc/services/instance.js @@ -1,25 +1,27 @@ import Controller from '@ember/controller'; import { get } from '@ember/object'; import { inject as service } from '@ember/service'; -import WithEventSource, { listen } from 'consul-ui/mixins/with-event-source'; -export default Controller.extend(WithEventSource, { +export default Controller.extend({ notify: service('flashMessages'), - item: listen('item').catch(function(e) { - if (e.target.readyState === 1) { - // OPEN - if (get(e, 'error.errors.firstObject.status') === '404') { - this.notify.add({ - destroyOnClick: false, - sticky: true, - type: 'warning', - action: 'update', - }); - const proxy = this.proxy; - if (proxy) { - proxy.close(); + actions: { + error: function(e) { + if (e.target.readyState === 1) { + // OPEN + if (get(e, 'error.errors.firstObject.status') === '404') { + this.notify.add({ + destroyOnClick: false, + sticky: true, + type: 'warning', + action: 'update', + }); + [e.target, this.proxy].forEach(function(item) { + if (item && typeof item.close === 'function') { + item.close(); + } + }); } } - } - }), + }, + }, }); diff --git a/ui-v2/app/controllers/dc/services/show.js b/ui-v2/app/controllers/dc/services/show.js index 161619206..04cc4bdf1 100644 --- a/ui-v2/app/controllers/dc/services/show.js +++ b/ui-v2/app/controllers/dc/services/show.js @@ -1,21 +1,29 @@ import Controller from '@ember/controller'; import { get } from '@ember/object'; import { inject as service } from '@ember/service'; -import WithEventSource, { listen } from 'consul-ui/mixins/with-event-source'; -export default Controller.extend(WithEventSource, { +export default Controller.extend({ dom: service('dom'), notify: service('flashMessages'), - item: listen('item').catch(function(e) { - if (e.target.readyState === 1) { - // OPEN - if (get(e, 'error.errors.firstObject.status') === '404') { - this.notify.add({ - destroyOnClick: false, - sticky: true, - type: 'warning', - action: 'update', - }); + actions: { + error: function(e) { + if (e.target.readyState === 1) { + // OPEN + if (get(e, 'error.errors.firstObject.status') === '404') { + this.notify.add({ + destroyOnClick: false, + sticky: true, + type: 'warning', + action: 'update', + }); + } + [e.target, this.intentions, this.chain, this.proxies, this.gatewayServices].forEach( + function(item) { + if (item && typeof item.close === 'function') { + item.close(); + } + } + ); } - } - }), + }, + }, }); diff --git a/ui-v2/app/instance-initializers/event-source.js b/ui-v2/app/instance-initializers/event-source.js index 599d3fddd..322ffad09 100644 --- a/ui-v2/app/instance-initializers/event-source.js +++ b/ui-v2/app/instance-initializers/event-source.js @@ -19,7 +19,7 @@ export function initialize(container) { }; }) .concat( - ['dc', 'policy', 'role'].map(function(item) { + ['policy', 'role'].map(function(item) { // create repositories that return a promise resolving to an EventSource return { service: `repository/${item}/component`, diff --git a/ui-v2/app/mixins/with-event-source.js b/ui-v2/app/mixins/with-event-source.js deleted file mode 100644 index 6e25a12f3..000000000 --- a/ui-v2/app/mixins/with-event-source.js +++ /dev/null @@ -1,61 +0,0 @@ -import Mixin from '@ember/object/mixin'; -import { set } from '@ember/object'; -import { computed as catchable } from 'consul-ui/computed/catchable'; -import purify from 'consul-ui/utils/computed/purify'; - -import WithListeners from 'consul-ui/mixins/with-listeners'; -const PREFIX = '_'; -export default Mixin.create(WithListeners, { - setProperties: function(model) { - const _model = {}; - Object.keys(model).forEach(prop => { - // here (see comment below on deleting) - if (model[prop] && typeof model[prop].addEventListener === 'function') { - let meta; - // TODO: metaForProperty throws an error if the property is not - // computed-like, this is far from ideal but happy with this - // until we can find a better way in an ember post 2.18 world - // of finding out if a property is computed or not - // (or until we switch all this out for compoments - try { - meta = this.constructor.metaForProperty(prop); - } catch (e) { - meta = {}; - } - if (typeof meta.catch === 'function') { - _model[`${PREFIX}${prop}`] = model[prop]; - this.listen(_model[`_${prop}`], 'error', meta.catch.bind(this)); - } else { - _model[prop] = model[prop]; - } - } else { - _model[prop] = model[prop]; - } - }); - return this._super(_model); - }, - reset: function(exiting) { - Object.keys(this).forEach(prop => { - if (this[prop] && typeof this[prop].close === 'function') { - this[prop].willDestroy(); - // ember doesn't delete on 'resetController' by default - // right now we only call reset when we are exiting, therefore a full - // setProperties will be called the next time we enter the Route so this - // is ok for what we need and means that the above conditional works - // as expected (see 'here' comment above) - // delete this[prop]; - // TODO: Check that nulling this out instead of deleting is fine - // pretty sure it is as above is just a falsey check - set(this, prop, null); - } - }); - return this._super(...arguments); - }, - willDestroy: function() { - this.reset(true); - this._super(...arguments); - }, -}); -export const listen = purify(catchable, function(props) { - return props.map(item => `${PREFIX}${item}`); -}); diff --git a/ui-v2/app/templates/dc/intentions/index.hbs b/ui-v2/app/templates/dc/intentions/index.hbs index ba2de5200..6f5197560 100644 --- a/ui-v2/app/templates/dc/intentions/index.hbs +++ b/ui-v2/app/templates/dc/intentions/index.hbs @@ -1,4 +1,5 @@ {{title 'Intentions'}} + {{#let (filter-by "Action" "deny" items) as |denied|}} {{#let (selectable-key-values (array "" (concat "All (" items.length ")")) diff --git a/ui-v2/app/templates/dc/nodes/index.hbs b/ui-v2/app/templates/dc/nodes/index.hbs index cdd7a27ca..3ecdf5cf9 100644 --- a/ui-v2/app/templates/dc/nodes/index.hbs +++ b/ui-v2/app/templates/dc/nodes/index.hbs @@ -1,4 +1,5 @@ {{title 'Nodes'}} + {{#let (selectable-key-values (array "" "All (Any Status)") (array "critical" "Critical Checks") diff --git a/ui-v2/app/templates/dc/nodes/show.hbs b/ui-v2/app/templates/dc/nodes/show.hbs index 90188bbca..624c5b67c 100644 --- a/ui-v2/app/templates/dc/nodes/show.hbs +++ b/ui-v2/app/templates/dc/nodes/show.hbs @@ -1,4 +1,5 @@ {{title item.Node}} + {{!TODO: Move sessions to its own folder within nodes }} diff --git a/ui-v2/app/templates/dc/nspaces/index.hbs b/ui-v2/app/templates/dc/nspaces/index.hbs index 311e03f4a..0e9fdc5c7 100644 --- a/ui-v2/app/templates/dc/nspaces/index.hbs +++ b/ui-v2/app/templates/dc/nspaces/index.hbs @@ -1,4 +1,5 @@ {{title 'Namespaces'}} + {{partial 'dc/nspaces/notifications'}} diff --git a/ui-v2/app/templates/dc/services/index.hbs b/ui-v2/app/templates/dc/services/index.hbs index 1fbacb08a..7c1ce8b29 100644 --- a/ui-v2/app/templates/dc/services/index.hbs +++ b/ui-v2/app/templates/dc/services/index.hbs @@ -1,4 +1,5 @@ {{title 'Services'}} + {{#let (selectable-key-values (array "Name:asc" "A to Z") (array "Name:desc" "Z to A") diff --git a/ui-v2/app/templates/dc/services/instance.hbs b/ui-v2/app/templates/dc/services/instance.hbs index a9ab9bb0b..cfe57a7ec 100644 --- a/ui-v2/app/templates/dc/services/instance.hbs +++ b/ui-v2/app/templates/dc/services/instance.hbs @@ -1,4 +1,6 @@ {{title item.ID}} + + {{partial 'dc/services/notifications'}} diff --git a/ui-v2/app/templates/dc/services/show.hbs b/ui-v2/app/templates/dc/services/show.hbs index 38d56af44..ccd9accfe 100644 --- a/ui-v2/app/templates/dc/services/show.hbs +++ b/ui-v2/app/templates/dc/services/show.hbs @@ -1,4 +1,7 @@ {{title item.Service.Service}} + + + {{partial 'dc/services/notifications'}} diff --git a/ui-v2/app/utils/computed/purify.js b/ui-v2/app/utils/computed/purify.js deleted file mode 100644 index 1008ed8c4..000000000 --- a/ui-v2/app/utils/computed/purify.js +++ /dev/null @@ -1,42 +0,0 @@ -import { get, computed } from '@ember/object'; - -/** - * Converts a conventional non-pure Ember `computed` function into a pure one - * (see https://github.com/emberjs/rfcs/blob/be351b059f08ac0fe709bc7697860d5064717a7f/text/0000-tracked-properties.md#avoiding-dependency-hell) - * - * @param {function} computed - a computed function to 'purify' (convert to a pure function) - * @param {function} filter - Optional string filter function to pre-process the names of computed properties - * @returns {function} - A pure `computed` function - */ -const _success = function(value) { - return value; -}; -const purify = function(computed, filter = args => args) { - return function() { - let args = [...arguments]; - let success = _success; - // pop the user function off the end - if (typeof args[args.length - 1] === 'function') { - success = args.pop(); - } - args = filter(args); - // this is the 'conventional' `computed` - const cb = function(name) { - return success.apply( - this, - args.map(item => { - // Right now this just takes the first part of the path so: - // `items.[]` or `items.@each.prop` etc - // gives you `items` which is 'probably' what you expect - // it won't work with something like `item.objects.[]` - // it could potentially be made to do so, but we don't need that right now at least - return get(this, item.split('.')[0]); - }) - ); - }; - // concat/push the user function back on - return computed(...args.concat([cb])); - }; -}; -export const subscribe = purify(computed); -export default purify; diff --git a/ui-v2/app/utils/dom/event-source/proxy.js b/ui-v2/app/utils/dom/event-source/proxy.js index a8213e241..eca66e184 100644 --- a/ui-v2/app/utils/dom/event-source/proxy.js +++ b/ui-v2/app/utils/dom/event-source/proxy.js @@ -4,36 +4,24 @@ const proxies = {}; export default function(ObjProxy, ArrProxy, createListeners) { return function(source, data = []) { let Proxy = ObjProxy; - // TODO: Why are these two separate? - // And when is data ever a string? - if (typeof data !== 'string' && typeof get(data, 'length') !== 'undefined') { - data = data.filter(function(item) { - return !get(item, 'isDestroyed') && !get(item, 'isDeleted') && get(item, 'isLoaded'); - }); - } + // TODO: When is data ever a string? let type = 'object'; if (typeof data !== 'string' && typeof get(data, 'length') !== 'undefined') { Proxy = ArrProxy; type = 'array'; + data = data.filter(function(item) { + return !get(item, 'isDestroyed') && !get(item, 'isDeleted') && get(item, 'isLoaded'); + }); } if (typeof proxies[type] === 'undefined') { proxies[type] = Proxy.extend({ - closed: false, - error: null, init: function() { this.listeners = createListeners(); this.listeners.add(this._source, 'message', e => set(this, 'content', e.data)); - this.listeners.add(this._source, 'open', () => set(this, 'closed', false)); - this.listeners.add(this._source, 'close', () => set(this, 'closed', true)); - this.listeners.add(this._source, 'error', e => set(this, 'error', e.error)); this._super(...arguments); }, addEventListener: function(type, handler) { - // Force use of computed for messages - // Temporarily disable this restriction - // if (type !== 'message') { this.listeners.add(this._source, type, handler); - // } }, getCurrentEvent: function() { return this._source.getCurrentEvent(...arguments); diff --git a/ui-v2/tests/integration/components/event-source-test.js b/ui-v2/tests/integration/components/event-source-test.js new file mode 100644 index 000000000..8b6a4d2a6 --- /dev/null +++ b/ui-v2/tests/integration/components/event-source-test.js @@ -0,0 +1,26 @@ +import { module, test } from 'qunit'; +import { setupRenderingTest } from 'ember-qunit'; +import { render } from '@ember/test-helpers'; +import { hbs } from 'ember-cli-htmlbars'; + +module('Integration | Component | event-source', function(hooks) { + setupRenderingTest(hooks); + + test('it renders', async function(assert) { + // Set any properties with this.set('myProperty', 'value'); + // Handle any actions with this.set('myAction', function(val) { ... }); + + await render(hbs``); + + assert.equal(this.element.textContent.trim(), ''); + + // Template block usage: + await render(hbs` + + template block text + + `); + + assert.equal(this.element.textContent.trim(), 'template block text'); + }); +});