[ML] Fix cleanup of mlAnomaliesTableService listeners in Time Series …

…Viewer. (elastic#25967) - A missing call to componentWillUnmount() in the Single Series Viewer didn't properly clean up the listeners for mlAnomaliesTableService so when switching to the Anomaly Explorer the page would crash if the user hovered the Anomaly Table. - This fixes it by calling ReactDOM.unmountComponentAtNode(element[0]); in element.on('$destroy', () => { ... }) to trigger the cleanup. - Additionally, as a safety measure, mlChartTooltipService.show() now silently fails if target is undefined.
walterra · Nov 21, 2018 · 77f29ef · 77f29ef
1 parent ac418b0
commit 77f29ef
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 1 deletion.
diff --git a/x-pack/plugins/ml/public/components/chart_tooltip/__tests__/chart_tooltip.js b/x-pack/plugins/ml/public/components/chart_tooltip/__tests__/chart_tooltip.js
@@ -15,4 +15,10 @@ describe('ML - mlChartTooltipService', () => {
     expect(mlChartTooltipService.hide).to.be.a('function');
   });
 
+  it('should fail silently when target is not defined', () => {
+    mlChartTooltipService.element = {};
+    expect(() => {
+      mlChartTooltipService.show('', undefined);
+    }).to.not.throwError('Call to show() should fail silently.');
+  });
 });
diff --git a/x-pack/plugins/ml/public/components/chart_tooltip/chart_tooltip_service.js b/x-pack/plugins/ml/public/components/chart_tooltip/chart_tooltip_service.js
@@ -18,7 +18,7 @@ export const mlChartTooltipService = {
 };
 
 mlChartTooltipService.show = function (contents, target, offset = { x: 0, y: 0 }) {
-  if (this.element === null) {
+  if (this.element === null || typeof target === 'undefined') {
     return;
   }
 

diff --git a/x-pack/plugins/ml/public/timeseriesexplorer/__tests__/timeseries_chart_directive.js b/x-pack/plugins/ml/public/timeseriesexplorer/__tests__/timeseries_chart_directive.js
@@ -0,0 +1,62 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import ngMock from 'ng_mock';
+import expect from 'expect.js';
+import sinon from 'sinon';
+
+import { TimeseriesChart } from '../timeseries_chart';
+
+describe('ML - <ml-timeseries-chart>', () => {
+  let $scope;
+  let $compile;
+  let $element;
+
+  beforeEach(ngMock.module('kibana'));
+  beforeEach(() => {
+    ngMock.inject(function ($injector) {
+      $compile = $injector.get('$compile');
+      const $rootScope = $injector.get('$rootScope');
+      $scope = $rootScope.$new();
+    });
+  });
+
+  afterEach(() => {
+    $scope.$destroy();
+  });
+
+  it('Plain initialization doesn\'t throw an error', () => {
+    // this creates a dummy DOM element with class 'ml-timeseries-chart' as a direct child of
+    // the <body> tag so the directive can find it in the DOM to create the resizeChecker.
+    const mockClassedElement = document.createElement('div');
+    mockClassedElement.classList.add('ml-timeseries-chart');
+    document.getElementsByTagName('body')[0].append(mockClassedElement);
+
+    // spy the TimeseriesChart component's unmount method to be able to test if it was called
+    const componentWillUnmountSpy = sinon.spy(TimeseriesChart.prototype, 'componentWillUnmount');
+
+    $element = $compile('<ml-timeseries-chart show-forecast="true" />')($scope);
+    const scope = $element.isolateScope();
+
+    // sanity test to check if directive picked up the attribute for its scope
+    expect(scope.showForecast).to.equal(true);
+
+    // componentWillUnmount() should not have been called so far
+    expect(componentWillUnmountSpy.callCount).to.equal(0);
+
+    // remove $element to trigger $destroy() callback
+    $element.remove();
+
+    // componentWillUnmount() should now have been called once
+    expect(componentWillUnmountSpy.callCount).to.equal(1);
+
+    componentWillUnmountSpy.restore();
+
+    // clean up the dummy DOM element
+    mockClassedElement.parentNode.removeChild(mockClassedElement);
+  });
+
+});
diff --git a/x-pack/plugins/ml/public/timeseriesexplorer/timeseries_chart_directive.js b/x-pack/plugins/ml/public/timeseriesexplorer/timeseries_chart_directive.js
@@ -90,6 +90,9 @@ module.directive('mlTimeseriesChart', function () {
 
     element.on('$destroy', () => {
       resizeChecker.destroy();
+      // unmountComponentAtNode() needs to be called so mlAnomaliesTableService listeners within
+      // the TimeseriesChart component get unwatched properly.
+      ReactDOM.unmountComponentAtNode(element[0]);
       scope.$destroy();
     });
-Original file line number
+Diff line change
@@ Expand Up / @@ -18,7 +18,7 @@ export const mlChartTooltipService = { @@
     };
     mlChartTooltipService.show = function (contents, target, offset = { x: 0, y: 0 }) {
-      if (this.element === null) {
+      if (this.element === null || typeof target === 'undefined') {
         return;
       }
@@ Expand Down @@